* [PATCH v3 3/5] tag: add format specifier to gpg_verify_tag
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas P
In-Reply-To: <20160930221806.3398-1-santiago@nyu.edu>
From: Lukas P <luk.puehringer@gmail.com>
Calling functions for gpg_verify_tag() may desire to print relevant
information about the header for further verification. Add an optional
format argument to print any desired information after GPG verification.
Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
builtin/tag.c | 2 +-
builtin/verify-tag.c | 2 +-
tag.c | 17 +++++++++++------
tag.h | 4 ++--
4 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 50e4ae5..14f3b48 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -105,7 +105,7 @@ static int delete_tag(const char *name, const char *ref,
static int verify_tag(const char *name, const char *ref,
const unsigned char *sha1)
{
- return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
+ return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
}
static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 99f8148..de10198 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -51,7 +51,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
const char *name = argv[i++];
if (get_sha1(name, sha1))
had_error = !!error("tag '%s' not found.", name);
- else if (gpg_verify_tag(sha1, name, flags))
+ else if (verify_and_format_tag(sha1, name, NULL, flags))
had_error = 1;
}
return had_error;
diff --git a/tag.c b/tag.c
index 291073f..d3512c0 100644
--- a/tag.c
+++ b/tag.c
@@ -4,6 +4,7 @@
#include "tree.h"
#include "blob.h"
#include "gpg-interface.h"
+#include "ref-filter.h"
const char *tag_type = "tag";
@@ -33,8 +34,8 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
return ret;
}
-int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
- unsigned flags)
+int verify_and_format_tag(const unsigned char *sha1, const char *name,
+ const char *fmt_pretty, unsigned flags)
{
enum object_type type;
char *buf;
@@ -44,21 +45,25 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
type = sha1_object_info(sha1, NULL);
if (type != OBJ_TAG)
return error("%s: cannot verify a non-tag object of type %s.",
- name_to_report ?
- name_to_report :
+ name ?
+ name :
find_unique_abbrev(sha1, DEFAULT_ABBREV),
typename(type));
buf = read_sha1_file(sha1, &type, &size);
if (!buf)
return error("%s: unable to read file.",
- name_to_report ?
- name_to_report :
+ name ?
+ name :
find_unique_abbrev(sha1, DEFAULT_ABBREV));
ret = run_gpg_verify(buf, size, flags);
free(buf);
+
+ if (fmt_pretty)
+ pretty_print_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS);
+
return ret;
}
diff --git a/tag.h b/tag.h
index a5721b6..896b9c2 100644
--- a/tag.h
+++ b/tag.h
@@ -17,7 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si
extern int parse_tag(struct tag *item);
extern struct object *deref_tag(struct object *, const char *, int);
extern struct object *deref_tag_noverify(struct object *);
-extern int gpg_verify_tag(const unsigned char *sha1,
- const char *name_to_report, unsigned flags);
+extern int verify_and_format_tag(const unsigned char *sha1, const char *name,
+ const char *fmt_pretty, unsigned flags);
#endif /* TAG_H */
--
2.10.0
^ permalink raw reply related
* [PATCH v3 2/5] ref-filter: add function to print single ref_array_item
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20160930221806.3398-1-santiago@nyu.edu>
From: Lukas Puehringer <luk.puehringer@gmail.com>
ref-filter functions are useful for printing git object information
using a format specifier. However, some other modules may not want to use
this functionality on a ref-array but only print a single item.
Expose a format_ref function to create, pretty print and free individual
ref-items.
Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
ref-filter.c | 10 ++++++++++
ref-filter.h | 3 +++
2 files changed, 13 insertions(+)
diff --git a/ref-filter.c b/ref-filter.c
index bc551a7..ee3ed67 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1655,6 +1655,16 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
putchar('\n');
}
+void pretty_print_ref(const char *name, const unsigned char *sha1,
+ const char *format, unsigned kind)
+{
+ struct ref_array_item *ref_item;
+ ref_item = new_ref_array_item(name, sha1, 0);
+ ref_item->kind = kind;
+ show_ref_array_item(ref_item, format, 0);
+ free_array_item(ref_item);
+}
+
/* If no sorting option is given, use refname to sort as default */
struct ref_sorting *ref_default_sorting(void)
{
diff --git a/ref-filter.h b/ref-filter.h
index 14d435e..3d23090 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void);
/* Function to parse --merged and --no-merged options */
int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
+void pretty_print_ref(const char *name, const unsigned char *sha1,
+ const char *format, unsigned kind);
+
#endif /* REF_FILTER_H */
--
2.10.0
^ permalink raw reply related
* [PATCH v3 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20160930221806.3398-1-santiago@nyu.edu>
From: Lukas Puehringer <luk.puehringer@gmail.com>
Functions that print git object information may require that the
gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent
print_signature_buffer from being called if flag is set.
Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
gpg-interface.h | 1 +
tag.c | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/gpg-interface.h b/gpg-interface.h
index ea68885..85dc982 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -3,6 +3,7 @@
#define GPG_VERIFY_VERBOSE 1
#define GPG_VERIFY_RAW 2
+#define GPG_VERIFY_QUIET 4
struct signature_check {
char *payload;
diff --git a/tag.c b/tag.c
index d1dcd18..291073f 100644
--- a/tag.c
+++ b/tag.c
@@ -3,6 +3,7 @@
#include "commit.h"
#include "tree.h"
#include "blob.h"
+#include "gpg-interface.h"
const char *tag_type = "tag";
@@ -24,7 +25,9 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
ret = check_signature(buf, payload_size, buf + payload_size,
size - payload_size, &sigc);
- print_signature_buffer(&sigc, flags);
+
+ if (!(flags & GPG_VERIFY_QUIET))
+ print_signature_buffer(&sigc, flags);
signature_check_clear(&sigc);
return ret;
--
2.10.0
^ permalink raw reply related
* [PATCH v3 0/5] Add --format to tag verification
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
From: Santiago Torres <santiago@nyu.edu>
This is the third iteration of [1][2], and as a result of the discussion
in [3].
In this re-roll we:
* Fixed all the signed-off-by's
[0002]
* Renamed the function format_ref to pretty_print_ref instead, which
is a more descriptive name
[0004]
* Added the respective line for the new --format parameter in the
documentation.
[0005]
* Added mention of the --format flag in the documentation files.
* Fixed the function signatures, now they take an opaque void *cb_data pointer
so it can be used in a more general way (by e.g., delete_tag).
This patch applies to 2.10.0 and master.
[1] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/
[2] http://public-inbox.org/git/20160926224233.32702-1-santiago@nyu.edu/
[3] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/
Lukas Puehringer (4):
gpg-interface, tag: add GPG_VERIFY_QUIET flag
ref-filter: add function to print single ref_array_item
tag: add format specifier to gpg_verify_tag
builtin/tag: add --format argument for tag -v
Santiago Torres (1):
builtin/verify-tag: add --format to verify-tag
Documentation/git-tag.txt | 2 +-
Documentation/git-verify-tag.txt | 2 +-
builtin/tag.c | 34 +++++++++++++++++++++++-----------
builtin/verify-tag.c | 13 +++++++++++--
gpg-interface.h | 1 +
ref-filter.c | 10 ++++++++++
ref-filter.h | 3 +++
tag.c | 22 +++++++++++++++-------
tag.h | 4 ++--
9 files changed, 67 insertions(+), 24 deletions(-)
--
2.10.0
^ permalink raw reply
* Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories
From: Jeff King @ 2016-09-30 22:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, David Turner
In-Reply-To: <xmqqponl84h4.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 30, 2016 at 02:25:43PM -0700, Junio C Hamano wrote:
> > +void add_to_alternates_internal(const char *reference)
> > +{
> > + prepare_alt_odb();
> > + link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0);
> > +}
> > +
>
> A function _internal being extern felt a bit funny. We are only
> appending so the first one does not have to be reprepare.
It's a match for add_to_alternates_file(). Suggestions for a better word
are welcome.
We do need to prepare_alt_odb(), as that is what sets up the
alt_odb_tail pointer. And also, a later prepare() call would overwrite
our entry. We could refactor the alt_odb code, but it seemed simplest
to just make sure we don't add to an unprepared list.
> > + t = xmalloc(sizeof(*t));
> > + strbuf_init(&t->path, 0);
> > + argv_array_init(&t->env);
> > +
> > + strbuf_addf(&t->path, "%s/incoming-XXXXXX", get_object_directory());
>
> I was wondering where you would put this in. Inside .git/objects/
> sounds good.
The name "incoming" is kind of arbitrary and related to the fact that
this is used for receive-pack (though if we were to use it on the
fetching side, I think it would be equally correct). I don't think it
really matters in practice.
> > +static int pack_copy_priority(const char *name)
> > +{
> > + if (!starts_with(name, "pack"))
> > + return 0;
> > + if (ends_with(name, ".keep"))
> > + return 1;
> > + if (ends_with(name, ".pack"))
> > + return 2;
> > + if (ends_with(name, ".idx"))
> > + return 3;
> > + return 4;
> > +}
>
> Thanks for being careful. A blind "cp -r" would have ruined the
> day.
>
> We do not do bitmaps upon receiving, I guess.
But we don't, but they (and anything else) would just sort at the end,
which is OK.
> > + * struct tmp_objdir *t = tmp_objdir_create();
> > + * if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
> > + * !tmp_objdir_migrate(t))
> > + * printf("success!\n");
> > + * else
> > + * die("failed...tmp_objdir will clean up for us");
>
> Made me briefly wonder if a caller might want to use appropriate
> environment to use the tmp-objdir given by the API in addition to
> its own, but then such a caller just needs to prepare its own argv-array
> and concatenate tmp_objdir_env() before making the opt_cd_env call,
> so this is perfectly fine.
Yep, and that's exactly what happens in one spot of the next patch.
My original had just open-coded, but I was happy to see we have
argv_array_pushv() these days, so it's a one-liner.
In the very original version, the receive-pack process did not need to
access the new objects at all (not until ref update time anyway, at
which point they've been migrated). And that's why the environment is
intentionally kept separate, and the caller can feed it to whichever
sub-programs it chooses. But a later version of git that handled shallow
pushes required receive-pack to actually look at the objects, and I
added the add_to_alternates_internal() call you see here.
At that point, it does make me wonder if a better interface would be for
tmp_objdir to just set up the environment variables in the parent
process in the first place, and then restore them upon
tmp_objdir_destroy(). It makes things a bit more automatic, which makes
me hesitate, but I think it would be fine for receive-pack.
I dunno. I mostly left it alone because I did it this way long ago, and
it wasn't broke. Polishing for upstream is an opportunity to fix old
oddities, but I think there is some value in applying a more
battle-tested patch.
-Peff
^ permalink raw reply
* Re: [PATCH 1/5] pretty: allow formatting DATE_SHORT
From: Jacob Keller @ 2016-09-30 22:04 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Jeff King, Kyle J. McKay, Git mailing list, Junio C Hamano
In-Reply-To: <20160930105639.15589-1-szeder@ira.uka.de>
On Fri, Sep 30, 2016 at 3:56 AM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> On Thu, Sep 29, 2016 at 1:33 AM, Jeff King <peff@peff.net> wrote:
>> > There's no way to do this short of "%ad" and --date=short,
>> > but that limits you to having a single date format in the
>> > output.
>> >
>> > This would possibly be better done with something more like
>> > "%ad(short)".
>> >
>> > Signed-off-by: Jeff King <peff@peff.net>
>> > ---
>> > pretty.c | 3 +++
>> > 1 file changed, 3 insertions(+)
>> >
>> > diff --git a/pretty.c b/pretty.c
>> > index 493edb0..c532c17 100644
>> > --- a/pretty.c
>> > +++ b/pretty.c
>> > @@ -727,6 +727,9 @@ static size_t format_person_part(struct strbuf *sb, char part,
>> > case 'I': /* date, ISO 8601 strict */
>> > strbuf_addstr(sb, show_ident_date(&s, DATE_MODE(ISO8601_STRICT)));
>> > return placeholder_len;
>> > + case 's':
>> > + strbuf_addstr(sb, show_ident_date(&s, DATE_MODE(SHORT)));
>> > + return placeholder_len;
>> > }
>> >
>> > skip:
>> > --
>> > 2.10.0.566.g5365f87
>> >
>>
>> Nice. I use date=short in some of my aliases and switching to this is
>> nicer. I assume this turns into "%(as)"?
>>
>> What about documenting this in pretty-formats.txt?
>
> Here you go :)
>
> http://public-inbox.org/git/1444235305-8718-1-git-send-email-szeder@ira.uka.de/
>
Nice, thanks!
Regards,
Jake
^ permalink raw reply
* RE: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories
From: David Turner @ 2016-09-30 21:32 UTC (permalink / raw)
To: 'Jeff King', git@vger.kernel.org
In-Reply-To: <20160930193613.dwpjiw5xps6a3wgj@sigill.intra.peff.net>
> +static void env_append(struct argv_array *env, const char *key, const
> +char *val) {
> + const char *old = getenv(key);
> +
> + if (!old)
> + argv_array_pushf(env, "%s=%s", key, val);
> + else
> + argv_array_pushf(env, "%s=%s%c%s", key, old, PATH_SEP,
> val);
>+}
I would like a comment explaining this function.
> + * Finalize a temporary object directory by migrating its objects into
> +the main
> + * object database.
> + */
This should mention that it frees its argument.
^ permalink raw reply
* Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories
From: Junio C Hamano @ 2016-09-30 21:25 UTC (permalink / raw)
To: Jeff King; +Cc: git, David Turner
In-Reply-To: <20160930193613.dwpjiw5xps6a3wgj@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/sha1_file.c b/sha1_file.c
> index 9a79c19..65deaf9 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -421,6 +421,12 @@ void add_to_alternates_file(const char *reference)
> free(alts);
> }
>
> +void add_to_alternates_internal(const char *reference)
> +{
> + prepare_alt_odb();
> + link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0);
> +}
> +
A function _internal being extern felt a bit funny. We are only
appending so the first one does not have to be reprepare.
> +static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)
> +{
> + int err;
> +
> + if (!t)
> + return 0;
> +
> + if (t == the_tmp_objdir)
> + the_tmp_objdir = NULL;
> +
> + /*
> + * This may use malloc via strbuf_grow(), but we should
> + * have pre-grown t->path sufficiently so that this
> + * doesn't happen in practice.
> + */
> + err = remove_dir_recursively(&t->path, 0);
> +
> + /*
> + * When we are cleaning up due to a signal, we won't bother
> + * freeing memory; it may cause a deadlock if the signal
> + * arrived while libc's allocator lock is held.
> + */
> + if (!on_signal)
> + tmp_objdir_free(t);
> + return err;
> +}
> +
> +int tmp_objdir_destroy(struct tmp_objdir *t)
> +{
> + return tmp_objdir_destroy_1(t, 0);
> +}
Looks sensible.
> + t = xmalloc(sizeof(*t));
> + strbuf_init(&t->path, 0);
> + argv_array_init(&t->env);
> +
> + strbuf_addf(&t->path, "%s/incoming-XXXXXX", get_object_directory());
I was wondering where you would put this in. Inside .git/objects/
sounds good.
> +/*
> + * Make sure we copy packfiles and their associated metafiles in the correct
> + * order. All of these ends_with checks are slightly expensive to do in
> + * the midst of a sorting routine, but in practice it shouldn't matter.
> + * We will have a relatively small number of packfiles to order, and loose
> + * objects exit early in the first line.
> + */
> +static int pack_copy_priority(const char *name)
> +{
> + if (!starts_with(name, "pack"))
> + return 0;
> + if (ends_with(name, ".keep"))
> + return 1;
> + if (ends_with(name, ".pack"))
> + return 2;
> + if (ends_with(name, ".idx"))
> + return 3;
> + return 4;
> +}
Thanks for being careful. A blind "cp -r" would have ruined the
day.
We do not do bitmaps upon receiving, I guess.
> + * struct tmp_objdir *t = tmp_objdir_create();
> + * if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
> + * !tmp_objdir_migrate(t))
> + * printf("success!\n");
> + * else
> + * die("failed...tmp_objdir will clean up for us");
Made me briefly wonder if a caller might want to use appropriate
environment to use the tmp-objdir given by the API in addition to
its own, but then such a caller just needs to prepare its own argv-array
and concatenate tmp_objdir_env() before making the opt_cd_env call,
so this is perfectly fine.
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Junio C Hamano @ 2016-09-30 20:49 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Christian Couder
In-Reply-To: <xmqq8tu99o75.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>>> I vaguely recall that there were some discussion on the definition
>>> of "what's a trailer line" with folks from the kernel land, perhaps
>>> while discussing the interpret-trailers topic. IIRC, when somebody
>>> passes an improved version along, the resulting message's trailer
>>> block may look like this:
>>>
>>> Signed-off-by: Original Author <original@author.xz>
>>> [fixed typo in the variable names]
>>> Signed-off-by: Somebhody Else <somebody@else.xz>
>>>
>>> and an obvious "wish" of theirs was to treat not just RFC2822-like
>>> "a line that begins with token followed by a colon" but also these
>>> short comments as part of the trailer block. Your original wish in
>>> [*1*] is to also treat "a line that begin with a whitespace that
>>> follows a line that begins with token followed by a colon" as part
>>> of the trailer block and I personally think that is a reasonable
>>> thing to wish for, too.
>>
>> If we allowed arbitrary lines in the trailer block, this would solve
>> my original problem, yes.
Here is an experiment I ran during my lunch break. The script
(attached) is meant to run in the kernel repository and
for each log messages of each non-merge commit:
* find its last paragraph, where the definition of paragraph is
simply "a blank/empty line";
* inspect if there is at least one RFC2822-header-looking line, or
a line that begins with "(cherry picked from";
* dump the ones that do not pass the above criteria.
My cursory look of the output did not spot a legitimate trailer
block that we should have identified. The output lines shown were
ones that are not signed off at all (e.g. af8c34ce6ae32add that says
"Linux 4.7-rc2"), ones that has three-dash line "---" in them
(e.g. 133d558216d9), ones that has diffstat that should have been
after "---" (e.g. 259307074bfcf1f).
The story is the same if you run it in git.git; the "do we have at
least one rfc2822-header-looking line or '(cherry picked from' line
in the last paragraph? if so, then that is an existing trailer
block" seems to be a good heuristics to cover many cases like
these:
d0196c8d5d3057c5c21a82f3d0113ca8e501033b
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
[tomi.valkeinen@ti.com: resolved conflicts]
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
59f0aa9480cfef9173a648cec4537addc5f3ad94
Link 1: https://bugzilla.kernel.org/show_bug.cgi?id=9916
http://bugzilla.kernel.org/show_bug.cgi?id=10100
https://lkml.org/lkml/2008/2/25/282
Link 2: https://bugzilla.kernel.org/show_bug.cgi?id=9399
https://bugzilla.kernel.org/show_bug.cgi?id=12461
https://bugzilla.kernel.org/show_bug.cgi?id=11880
Link 3: https://bugzilla.kernel.org/show_bug.cgi?id=11884
https://bugzilla.kernel.org/show_bug.cgi?id=14081
https://bugzilla.kernel.org/show_bug.cgi?id=14086
https://bugzilla.kernel.org/show_bug.cgi?id=14446
Link 4: https://bugzilla.kernel.org/show_bug.cgi?id=112911
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
Tested-by: Chris Bainbridge <chris.bainbridge@gmail.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
-- >8 --
#!/bin/sh
git log --no-merges |
perl -e '
sub flush {
my ($commit, @lines) = @_;
my $seen_good = 0;
for (@lines) {
if (/^[-A-Za-z0-9]+: / ||
/^\(cherry picked from/) {
$seen_good = 1;
last;
}
}
if (!$seen_good) {
print "\n$commit\n";
for (@lines) {
print;
}
}
}
my (@lines, $this);
while (<>) {
if (/^commit (.*)$/) {
my $next = $1;
flush($this, @lines);
@lines = ();
$this = $next;
}
if (s/^ //) {
if (/^\s*$/) {
@lines = ();
} else {
push @lines, $_;
}
}
}
if (@lines && $this) {
flush($this, @lines);
}
'
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Jonathan Tan @ 2016-09-30 20:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
In-Reply-To: <xmqq8tu99o75.fsf@gitster.mtv.corp.google.com>
On 09/30/2016 12:34 PM, Junio C Hamano wrote:
>> 2) The Linux kernel's repository has some "commit ... upstream." lines
>> in this position (below the commit title) - for example, in commit
>> dacc0987fd2e.
>
> "A group of people seem to prefer it there" does not lead to
> "therefore let's move it there for everybody". It does open a
> possibility that we may want to add a new option to put it there,
> but does not justify changing what existing "-x" option does.
To clarify, my patch adds the new option you described (to place it
below the title instead of at the bottom of the commit message). The
default is still the current behavior.
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-30 20:01 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFxyF=xX84AXr8MG14MRHwdrQw00PBM20UfqBdidaeqdMg@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Fri, Sep 30, 2016 at 10:54 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>>> So IMHO, the best combination is the init_default_abbrev() you posted in
>>> [1], but initialized at the top of find_unique_abbrev(). And cached
>>> there, obviously, in a similar way.
>>
>> That's certainly possible, but I'm really not happy with how the
>> counting function looks. And nobody actually stood up to say "yeah,
>> that gets alternate loose objects right" or "if you have tons of those
>> alternate loose objects you have other issues anyway". I think
>> somebody would have to "own" that counting function, the advantage of
>> just putting it into disambiguate_state is that we just get the
>> counting for free..
>
> Side note: maybe we can mix the two approaches, and keep the counting
> in the disambiguation state, and just make the counting function do
>
> init_object_disambiguation();
> find_short_object_filename(&ds);
> find_short_packed_object(&ds);
> finish_object_disambiguation(&ds, sha1);
>
> and then just use "ds.nrobjects". So the counting would still be done
> by the disambiguation code, it just woudln't be in get_short_sha1().
>
> So here's another version that takes that approach. And if somebody
> (hint hint) wants to do the counting differently, they can perhaps
> send an incremental patch to do that.
>
> (This patch also contains the few setup issues Junio found with the
> new "default_abbrev is negative" model)
Sorry, but I do not quite see the point in the difference between
this one and your original that had a hook in get_short_sha1(), as
it seemed to me that Peff's objection was about the counting done in
find_short_object_filename() and find_short_packed_object(), which
is (understandably) still here.
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Ævar Arnfjörð Bjarmason @ 2016-09-30 19:41 UTC (permalink / raw)
To: Linus Torvalds
Cc: Mike Hommey, Junio C Hamano, Johannes Sixt, Git Mailing List,
Jeff King
In-Reply-To: <CA+55aFzjTB0peMDPoPA6JyeUy90x=Lh4qdfiYLNf6RQU3ey9Hg@mail.gmail.com>
On Fri, Sep 30, 2016 at 3:01 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Thu, Sep 29, 2016 at 5:56 PM, Mike Hommey <mh@glandium.org> wrote:
>>
>> OTOH, how often does one refer to trees or blobs with abbreviated sha1s?
>> Most of the time, you'd use abbreviated sha1s for commits. And the number
>> of commits in git and the kernel repositories are much lower than the
>> number of overall objects.
>
> See that whole other discussion about this. I agree. If we only ever
> worried about just commits, the abbreviation length wouldn't need to
> be grown nearly as aggressively. The current default would still be
> wrong for the kernel, but it wouldn't be as noticeably wrong, and
> updating it to 8 or 9 would be fine.
>
> That said, people argued against that too. We *do* end up having
> abbreviated SHA1's for blobs in the diff index. When I said that _I_
> neer use it, somebody piped up to say that they do.
>
> So I'd rather just keep the existing semantics (a hash is a hash is a
> hash), and just abbreviate at a sufficient point that we don't have to
> worry too much about disambiguating further by object type.
I work on a repo that's around the size of linux.git in every way
(commits, objects etc.), and growing twice as fast.
So I also see 8 or 9 digit abbreviations on a daily basis, even with
the current defaults core.abbrev, but I still think growing it so
aggressively is the wrong thing to do.
The fact that we have a core.abbrev option at all and nobody's talking
about getting rid of it entirely means we all acknowledge the UX
convenience of short SHA1s.
I don't think it's a good idea for such UX options to have defaults
that really only make sense for repositories at the very far end of
the bell curve, which is the case with linux.git and the repo I work
on.
Either way you're going to waste somebody's time. I think it's a
better trade-off that some kernel dev occasionally has to look at
Peff's new disambiguation output, than have the wast hordes of
everyday Git users have less screen real estate, need to recite longer
sha1s over the phone during outages (people do that), and any number
of other every day use cases.
I think if anything we should be talking about making the default
shorter & then have some clever auto-scaling by repository size as has
been discussed in this thread to deal with the repositories at the far
end of the bell curve.
^ permalink raw reply
* Re: [PATCH v8 11/11] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-09-30 19:38 UTC (permalink / raw)
To: Jakub Narębski
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <2a604438-b6cd-876d-0ec2-90027dea99b9@gmail.com>
> On 27 Sep 2016, at 17:37, Jakub Narębski <jnareb@gmail.com> wrote:
>
> Part second of the review of 11/11.
>
> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>
>> +
>> + if (!drv->process && (CAP_CLEAN & wanted_capability) && drv->clean)
>
> This is just a very minor nitpicking, but wouldn't it be easier
> to read with those checks reordered?
>
> + if ((wanted_capability & CAP_CLEAN) && !drv->process && drv->clean)
OK
>> +
>> + if (start_command(process)) {
>> + error("cannot fork to run external filter '%s'", cmd);
>> + kill_multi_file_filter(hashmap, entry);
>> + return NULL;
>> + }
>
> I guess there is a reason why we init hashmap entry, try to start
> external process, then kill entry of unable to start, instead of
> trying to start external process, and adding hashmap entry when
> we succeed?
Yes. This way I can reuse the kill_multi_file_filter() function.
>> +
>> + sigchain_push(SIGPIPE, SIG_IGN);
>
> I guess that this is here to handle errors writing to filter
> by ourself, isn't it?
Yes.
>> + error("external filter '%s' does not support long running filter protocol", cmd);
>
> We could have described the error here better.
>
> + error("external filter '%s' does not support filter protocol version 2", cmd);
OK
>> +static void read_multi_file_filter_values(int fd, struct strbuf *status) {
>
> This is more
>
> +static void read_multi_file_filter_status(int fd, struct strbuf *status) {
>
> It doesn't read arbitrary values, it examines 'metadata' from
> filter for "status=<foo>" lines.
True!
>> + if (pair[0] && pair[0]->len && pair[1]) {
>> + if (!strcmp(pair[0]->buf, "status=")) {
>> + strbuf_reset(status);
>> + strbuf_addbuf(status, pair[1]);
>> + }
>
> So it is last status=<foo> line wins behavior?
Correct.
>
>> + }
>
> Shouldn't we free 'struct strbuf **pair', maybe allocated by the
> strbuf_split_str() function, and reset to NULL?
True. strbuf_list_free() should be enough.
>>
>> + fflush(NULL);
>
> Why this fflush(NULL) is needed here?
This flushes all open output streams. The single filter does the same.
>>
>> + if (fd >= 0 && !src) {
>> + if (fstat(fd, &file_stat) == -1)
>> + return 0;
>> + len = xsize_t(file_stat.st_size);
>> + }
>
> Errr... is it necessary? The protocol no longer provides size=<n>
> hint, and neither uses such hint if provided.
We require the size in write_packetized_from_buf() later.
>> +
>> + err = strlen(filter_type) > PKTLINE_DATA_MAXLEN;
>> + if (err)
>> + goto done;
>
> Errr... this should never happen. We control which capabilities
> we pass, it can be only "clean" or "smudge", nothing else. Those
> would always be shorter than PKTLINE_DATA_MAXLEN.
>
> Never mind that that is "command=smudge\n" etc. that needs to
> be shorter that PKTLINE_DATA_MAXLEN!
>
> So, IMHO it should be at most assert, and needs to be corrected
> anyway.
OK!
> This should never happen, PATH_MAX everywhere is much shorter
> than PKTLINE_DATA_MAXLEN / LARGE_PACKET_MAX. Or is it?
>
> Anyway, we should probably explain or warn
>
> error("path name too long: '%s'", path);
OK
>> + /*
>> + * Something went wrong with the protocol filter.
>> + * Force shutdown and restart if another blob requires filtering!
>
> Is this exclamation mark '!' here necessary?
>
No.
Thanks,
Lars
^ permalink raw reply
* [PATCH 6/6] tmp-objdir: do not migrate files starting with '.'
From: Jeff King @ 2016-09-30 19:36 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net>
This avoids "." and "..", as we already do, but also leaves
room for index-pack to store extra data in the quarantine
area (e.g., for passing back any analysis to be read by the
pre-receive hook).
Signed-off-by: Jeff King <peff@peff.net>
---
tmp-objdir.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 9f53238..2181a42 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -181,7 +181,7 @@ static int read_dir_paths(struct string_list *out, const char *path)
return -1;
while ((de = readdir(dh)))
- if (!is_dot_or_dotdot(de->d_name))
+ if (de->d_name[0] != '.')
string_list_append(out, de->d_name);
closedir(dh);
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 5/6] tmp-objdir: put quarantine information in the environment
From: Jeff King @ 2016-09-30 19:36 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net>
The presence of the GIT_QUARANTINE_PATH variable lets any
called programs know that they're operating in a temporary
object directory (and where that directory is).
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 1 +
tmp-objdir.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/cache.h b/cache.h
index 607c9b5..fd81a6c 100644
--- a/cache.h
+++ b/cache.h
@@ -433,6 +433,7 @@ static inline enum object_type object_type(unsigned int mode)
#define GIT_GLOB_PATHSPECS_ENVIRONMENT "GIT_GLOB_PATHSPECS"
#define GIT_NOGLOB_PATHSPECS_ENVIRONMENT "GIT_NOGLOB_PATHSPECS"
#define GIT_ICASE_PATHSPECS_ENVIRONMENT "GIT_ICASE_PATHSPECS"
+#define GIT_QUARANTINE_ENVIRONMENT "GIT_QUARANTINE_PATH"
/*
* This environment variable is expected to contain a boolean indicating
diff --git a/tmp-objdir.c b/tmp-objdir.c
index c92e6cc..9f53238 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -140,6 +140,8 @@ struct tmp_objdir *tmp_objdir_create(void)
env_append(&t->env, ALTERNATE_DB_ENVIRONMENT,
absolute_path(get_object_directory()));
env_replace(&t->env, DB_ENVIRONMENT, absolute_path(t->path.buf));
+ env_replace(&t->env, GIT_QUARANTINE_ENVIRONMENT,
+ absolute_path(t->path.buf));
return t;
}
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 4/6] receive-pack: quarantine objects until pre-receive accepts
From: Jeff King @ 2016-09-30 19:36 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net>
When a client pushes objects to us, index-pack checks the
objects themselves and then installs them into place. If we
then reject the push due to a pre-receive hook, we cannot
just delete the packfile; other processes may be depending
on it. We have to do a normal reachability check at this
point via `git gc`.
But such objects may hang around for weeks due to the
gc.pruneExpire grace period. And worse, during that time
they may be exploded from the pack into inefficient loose
objects.
Instead, this patch teaches receive-pack to put the new
objects into a "quarantine" temporary directory. We make
these objects available to the connectivity check and to the
pre-receive hook, and then install them into place only if
it is successful (and otherwise remove them as tempfiles).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/receive-pack.c | 41 ++++++++++++++++++++++++++++++++++++++++-
t/t5547-push-quarantine.sh | 36 ++++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+), 1 deletion(-)
create mode 100755 t/t5547-push-quarantine.sh
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 896b16f..04ad909 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -20,6 +20,7 @@
#include "gpg-interface.h"
#include "sigchain.h"
#include "fsck.h"
+#include "tmp-objdir.h"
static const char * const receive_pack_usage[] = {
N_("git receive-pack <git-dir>"),
@@ -86,6 +87,8 @@ static enum {
} use_keepalive;
static int keepalive_in_sec = 5;
+struct tmp_objdir *tmp_objdir;
+
static enum deny_action parse_deny_action(const char *var, const char *value)
{
if (value) {
@@ -663,6 +666,9 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
} else
argv_array_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
+ if (tmp_objdir)
+ argv_array_pushv(&proc.env_array, tmp_objdir_env(tmp_objdir));
+
if (use_sideband) {
memset(&muxer, 0, sizeof(muxer));
muxer.proc = copy_to_sideband;
@@ -762,6 +768,7 @@ static int run_update_hook(struct command *cmd)
proc.stdout_to_stderr = 1;
proc.err = use_sideband ? -1 : 0;
proc.argv = argv;
+ proc.env = tmp_objdir_env(tmp_objdir);
code = start_command(&proc);
if (code)
@@ -833,6 +840,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
!delayed_reachability_test(si, i))
sha1_array_append(&extra, si->shallow->sha1[i]);
+ opt.env = tmp_objdir_env(tmp_objdir);
setup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);
if (check_connected(command_singleton_iterator, cmd, &opt)) {
rollback_lock_file(&shallow_lock);
@@ -1240,12 +1248,17 @@ static void set_connectivity_errors(struct command *commands,
for (cmd = commands; cmd; cmd = cmd->next) {
struct command *singleton = cmd;
+ struct check_connected_options opt = CHECK_CONNECTED_INIT;
+
if (shallow_update && si->shallow_ref[cmd->index])
/* to be checked in update_shallow_ref() */
continue;
+
+ opt.env = tmp_objdir_env(tmp_objdir);
if (!check_connected(command_singleton_iterator, &singleton,
- NULL))
+ &opt))
continue;
+
cmd->error_string = "missing necessary objects";
}
}
@@ -1428,6 +1441,7 @@ static void execute_commands(struct command *commands,
data.si = si;
opt.err_fd = err_fd;
opt.progress = err_fd && !quiet;
+ opt.env = tmp_objdir_env(tmp_objdir);
if (check_connected(iterate_receive_command_list, &data, &opt))
set_connectivity_errors(commands, si);
@@ -1444,6 +1458,19 @@ static void execute_commands(struct command *commands,
return;
}
+ /*
+ * Now we'll start writing out refs, which means the objects need
+ * to be in their final positions so that other processes can see them.
+ */
+ if (tmp_objdir_migrate(tmp_objdir) < 0) {
+ for (cmd = commands; cmd; cmd = cmd->next) {
+ if (!cmd->error_string)
+ cmd->error_string = "unable to migrate objects to permanent storage";
+ }
+ return;
+ }
+ tmp_objdir = NULL;
+
check_aliased_updates(commands);
free(head_name_to_free);
@@ -1639,6 +1666,18 @@ static const char *unpack(int err_fd, struct shallow_info *si)
argv_array_push(&child.args, alt_shallow_file);
}
+ tmp_objdir = tmp_objdir_create();
+ if (!tmp_objdir)
+ return "unable to create temporary object directory";
+ child.env = tmp_objdir_env(tmp_objdir);
+
+ /*
+ * Normally we just pass the tmp_objdir environment to the child
+ * processes that do the heavy lifting, but we may need to see these
+ * objects ourselves to set up shallow information.
+ */
+ tmp_objdir_add_as_alternate(tmp_objdir);
+
if (ntohl(hdr.hdr_entries) < unpack_limit) {
argv_array_pushl(&child.args, "unpack-objects", hdr_arg, NULL);
if (quiet)
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
new file mode 100755
index 0000000..1e5d32d
--- /dev/null
+++ b/t/t5547-push-quarantine.sh
@@ -0,0 +1,36 @@
+#!/bin/sh
+
+test_description='check quarantine of objects during push'
+. ./test-lib.sh
+
+test_expect_success 'create picky dest repo' '
+ git init --bare dest.git &&
+ write_script dest.git/hooks/pre-receive <<-\EOF
+ while read old new ref; do
+ test "$(git log -1 --format=%s $new)" = reject && exit 1
+ done
+ exit 0
+ EOF
+'
+
+test_expect_success 'accepted objects work' '
+ test_commit ok &&
+ git push dest.git HEAD &&
+ commit=$(git rev-parse HEAD) &&
+ git --git-dir=dest.git cat-file commit $commit
+'
+
+test_expect_success 'rejected objects are not installed' '
+ test_commit reject &&
+ commit=$(git rev-parse HEAD) &&
+ test_must_fail git push dest.git reject &&
+ test_must_fail git --git-dir=dest.git cat-file commit $commit
+'
+
+test_expect_success 'rejected objects are removed' '
+ echo "incoming-*" >expect &&
+ (cd dest.git/objects && echo incoming-*) >actual &&
+ test_cmp expect actual
+'
+
+test_done
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 3/6] tmp-objdir: introduce API for temporary object directories
From: Jeff King @ 2016-09-30 19:36 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net>
Once objects are added to the object database by a process,
they cannot easily be deleted, as we don't know what other
processes may have started referencing them. We have to
clean them up with git-gc, which will apply the usual
reachability and grace-period checks.
This patch provides an alternative: it helps callers create
a temporary directory inside the object directory, and a
temporary environment which can be passed to sub-programs to
ask them to write there (the original object directory
remains accessible as an alternate of the temporary one).
See tmp-objdir.h for details on the API.
Signed-off-by: Jeff King <peff@peff.net>
---
Makefile | 1 +
cache.h | 1 +
sha1_file.c | 6 ++
tmp-objdir.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
tmp-objdir.h | 53 ++++++++++++
5 files changed, 327 insertions(+)
create mode 100644 tmp-objdir.c
create mode 100644 tmp-objdir.h
diff --git a/Makefile b/Makefile
index 1aad150..4e3becb 100644
--- a/Makefile
+++ b/Makefile
@@ -831,6 +831,7 @@ LIB_OBJS += submodule-config.o
LIB_OBJS += symlinks.o
LIB_OBJS += tag.o
LIB_OBJS += tempfile.o
+LIB_OBJS += tmp-objdir.o
LIB_OBJS += trace.o
LIB_OBJS += trailer.o
LIB_OBJS += transport.o
diff --git a/cache.h b/cache.h
index ed3d5df..607c9b5 100644
--- a/cache.h
+++ b/cache.h
@@ -1389,6 +1389,7 @@ extern void prepare_alt_odb(void);
extern void read_info_alternates(const char * relative_base, int depth);
extern char *compute_alternate_path(const char *path, struct strbuf *err);
extern void add_to_alternates_file(const char *reference);
+extern void add_to_alternates_internal(const char *reference);
typedef int alt_odb_fn(struct alternate_object_database *, void *);
extern int foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index 9a79c19..65deaf9 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -421,6 +421,12 @@ void add_to_alternates_file(const char *reference)
free(alts);
}
+void add_to_alternates_internal(const char *reference)
+{
+ prepare_alt_odb();
+ link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0);
+}
+
/*
* Compute the exact path an alternate is at and returns it. In case of
* error NULL is returned and the human readable error is added to `err`
diff --git a/tmp-objdir.c b/tmp-objdir.c
new file mode 100644
index 0000000..c92e6cc
--- /dev/null
+++ b/tmp-objdir.c
@@ -0,0 +1,266 @@
+#include "cache.h"
+#include "tmp-objdir.h"
+#include "dir.h"
+#include "sigchain.h"
+#include "string-list.h"
+#include "strbuf.h"
+#include "argv-array.h"
+
+struct tmp_objdir {
+ struct strbuf path;
+ struct argv_array env;
+};
+
+/*
+ * Allow only one tmp_objdir at a time in a running process, which simplifies
+ * our signal/atexit cleanup routines. It's doubtful callers will ever need
+ * more than one, and we can expand later if so. You can have many such
+ * tmp_objdirs simultaneously in many processes, of course.
+ */
+struct tmp_objdir *the_tmp_objdir;
+
+static void tmp_objdir_free(struct tmp_objdir *t)
+{
+ strbuf_release(&t->path);
+ argv_array_clear(&t->env);
+ free(t);
+}
+
+static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)
+{
+ int err;
+
+ if (!t)
+ return 0;
+
+ if (t == the_tmp_objdir)
+ the_tmp_objdir = NULL;
+
+ /*
+ * This may use malloc via strbuf_grow(), but we should
+ * have pre-grown t->path sufficiently so that this
+ * doesn't happen in practice.
+ */
+ err = remove_dir_recursively(&t->path, 0);
+
+ /*
+ * When we are cleaning up due to a signal, we won't bother
+ * freeing memory; it may cause a deadlock if the signal
+ * arrived while libc's allocator lock is held.
+ */
+ if (!on_signal)
+ tmp_objdir_free(t);
+ return err;
+}
+
+int tmp_objdir_destroy(struct tmp_objdir *t)
+{
+ return tmp_objdir_destroy_1(t, 0);
+}
+
+static void remove_tmp_objdir(void)
+{
+ tmp_objdir_destroy(the_tmp_objdir);
+}
+
+static void remove_tmp_objdir_on_signal(int signo)
+{
+ tmp_objdir_destroy_1(the_tmp_objdir, 1);
+ sigchain_pop(signo);
+ raise(signo);
+}
+
+static void env_append(struct argv_array *env, const char *key, const char *val)
+{
+ const char *old = getenv(key);
+
+ if (!old)
+ argv_array_pushf(env, "%s=%s", key, val);
+ else
+ argv_array_pushf(env, "%s=%s%c%s", key, old, PATH_SEP, val);
+}
+
+static void env_replace(struct argv_array *env, const char *key, const char *val)
+{
+ argv_array_pushf(env, "%s=%s", key, val);
+}
+
+static int setup_tmp_objdir(const char *root)
+{
+ char *path;
+ int ret = 0;
+
+ path = xstrfmt("%s/pack", root);
+ ret = mkdir(path, 0777);
+ free(path);
+
+ return ret;
+}
+
+struct tmp_objdir *tmp_objdir_create(void)
+{
+ static int installed_handlers;
+ struct tmp_objdir *t;
+
+ if (the_tmp_objdir)
+ die("BUG: only one tmp_objdir can be used at a time");
+
+ t = xmalloc(sizeof(*t));
+ strbuf_init(&t->path, 0);
+ argv_array_init(&t->env);
+
+ strbuf_addf(&t->path, "%s/incoming-XXXXXX", get_object_directory());
+
+ /*
+ * Grow the strbuf beyond any filename we expect to be placed in it.
+ * If tmp_objdir_destroy() is called by a signal handler, then
+ * we should be able to use the strbuf to remove files without
+ * having to call malloc.
+ */
+ strbuf_grow(&t->path, 1024);
+
+ if (!mkdtemp(t->path.buf)) {
+ /* free, not destroy, as we never touched the filesystem */
+ tmp_objdir_free(t);
+ return NULL;
+ }
+
+ the_tmp_objdir = t;
+ if (!installed_handlers) {
+ atexit(remove_tmp_objdir);
+ sigchain_push_common(remove_tmp_objdir_on_signal);
+ installed_handlers++;
+ }
+
+ if (setup_tmp_objdir(t->path.buf)) {
+ tmp_objdir_destroy(t);
+ return NULL;
+ }
+
+ env_append(&t->env, ALTERNATE_DB_ENVIRONMENT,
+ absolute_path(get_object_directory()));
+ env_replace(&t->env, DB_ENVIRONMENT, absolute_path(t->path.buf));
+
+ return t;
+}
+
+/*
+ * Make sure we copy packfiles and their associated metafiles in the correct
+ * order. All of these ends_with checks are slightly expensive to do in
+ * the midst of a sorting routine, but in practice it shouldn't matter.
+ * We will have a relatively small number of packfiles to order, and loose
+ * objects exit early in the first line.
+ */
+static int pack_copy_priority(const char *name)
+{
+ if (!starts_with(name, "pack"))
+ return 0;
+ if (ends_with(name, ".keep"))
+ return 1;
+ if (ends_with(name, ".pack"))
+ return 2;
+ if (ends_with(name, ".idx"))
+ return 3;
+ return 4;
+}
+
+static int pack_copy_cmp(const char *a, const char *b)
+{
+ return pack_copy_priority(a) - pack_copy_priority(b);
+}
+
+static int read_dir_paths(struct string_list *out, const char *path)
+{
+ DIR *dh;
+ struct dirent *de;
+
+ dh = opendir(path);
+ if (!dh)
+ return -1;
+
+ while ((de = readdir(dh)))
+ if (!is_dot_or_dotdot(de->d_name))
+ string_list_append(out, de->d_name);
+
+ closedir(dh);
+ return 0;
+}
+
+static int migrate_paths(struct strbuf *src, struct strbuf *dst);
+
+static int migrate_one(struct strbuf *src, struct strbuf *dst)
+{
+ struct stat st;
+
+ if (stat(src->buf, &st) < 0)
+ return -1;
+ if (S_ISDIR(st.st_mode)) {
+ if (!mkdir(dst->buf, 0777)) {
+ if (adjust_shared_perm(dst->buf))
+ return -1;
+ } else if (errno != EEXIST)
+ return -1;
+ return migrate_paths(src, dst);
+ }
+ return finalize_object_file(src->buf, dst->buf);
+}
+
+static int migrate_paths(struct strbuf *src, struct strbuf *dst)
+{
+ size_t src_len = src->len, dst_len = dst->len;
+ struct string_list paths = STRING_LIST_INIT_DUP;
+ int i;
+ int ret = 0;
+
+ if (read_dir_paths(&paths, src->buf) < 0)
+ return -1;
+ paths.cmp = pack_copy_cmp;
+ string_list_sort(&paths);
+
+ for (i = 0; i < paths.nr; i++) {
+ const char *name = paths.items[i].string;
+
+ strbuf_addf(src, "/%s", name);
+ strbuf_addf(dst, "/%s", name);
+
+ ret |= migrate_one(src, dst);
+
+ strbuf_setlen(src, src_len);
+ strbuf_setlen(dst, dst_len);
+ }
+
+ string_list_clear(&paths, 0);
+ return ret;
+}
+
+int tmp_objdir_migrate(struct tmp_objdir *t)
+{
+ struct strbuf src = STRBUF_INIT, dst = STRBUF_INIT;
+ int ret;
+
+ if (!t)
+ return 0;
+
+ strbuf_addbuf(&src, &t->path);
+ strbuf_addstr(&dst, get_object_directory());
+
+ ret = migrate_paths(&src, &dst);
+
+ strbuf_release(&src);
+ strbuf_release(&dst);
+
+ tmp_objdir_destroy(t);
+ return ret;
+}
+
+const char **tmp_objdir_env(const struct tmp_objdir *t)
+{
+ if (!t)
+ return NULL;
+ return t->env.argv;
+}
+
+void tmp_objdir_add_as_alternate(const struct tmp_objdir *t)
+{
+ add_to_alternates_internal(t->path.buf);
+}
diff --git a/tmp-objdir.h b/tmp-objdir.h
new file mode 100644
index 0000000..aa47aa9
--- /dev/null
+++ b/tmp-objdir.h
@@ -0,0 +1,53 @@
+#ifndef TMP_OBJDIR_H
+#define TMP_OBJDIR_H
+
+/*
+ * This API allows you to create a temporary object directory, advertise it to
+ * sub-processes via GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES,
+ * and then either migrate its object into the main object directory, or remove
+ * it. The library handles unexpected signal/exit death by cleaning up the
+ * temporary directory.
+ *
+ * Example:
+ *
+ * struct tmp_objdir *t = tmp_objdir_create();
+ * if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
+ * !tmp_objdir_migrate(t))
+ * printf("success!\n");
+ * else
+ * die("failed...tmp_objdir will clean up for us");
+ *
+ */
+
+struct tmp_objdir;
+
+/*
+ * Create a new temporary object directory; returns NULL on failure.
+ */
+struct tmp_objdir *tmp_objdir_create(void);
+
+/*
+ * Return a list of environment strings, suitable for use with
+ * child_process.env, that can be passed to child programs to make use of the
+ * temporary object directory.
+ */
+const char **tmp_objdir_env(const struct tmp_objdir *);
+
+/*
+ * Finalize a temporary object directory by migrating its objects into the main
+ * object database.
+ */
+int tmp_objdir_migrate(struct tmp_objdir *);
+
+/*
+ * Destroy a temporary object directory, discarding any objects it contains.
+ */
+int tmp_objdir_destroy(struct tmp_objdir *);
+
+/*
+ * Add the temporary object directory as an alternate object store in the
+ * current process.
+ */
+void tmp_objdir_add_as_alternate(const struct tmp_objdir *);
+
+#endif /* TMP_OBJDIR_H */
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 2/6] sha1_file: always allow relative paths to alternates
From: Jeff King @ 2016-09-30 19:36 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net>
We recursively expand alternates repositories, so that if A
borrows from B which borrows from C, A can see all objects.
For the root object database, we allow relative paths, so A
can point to B as "../B/objects". However, we currently do
not allow relative paths when recursing, so B must use an
absolute path to reach C.
That is an ancient protection from c2f493a (Transitively
read alternatives, 2006-05-07) that tries to avoid adding
the same alternate through two different paths. But since
5bdf0a8 (sha1_file: normalize alt_odb path before comparing
and storing, 2011-09-07), we use a normalized absolute path
for each alt_odb entry.
So this protection is no longer necessary; we will detect
the duplicate no matter how we got there. And it's a good
idea to get rid of it, as it creates an unnecessary
complication when setting up recursive alternates (B has to
know that A is going to borrow from it and make sure to use
an absolute path).
We adjust the test script here to demonstrate that this now
works. Unfortunately, we can't demonstrate that the
duplicate is suppressed, since it has no user-visible
behavior (it's just one less place for our object lookups to
go). But you can verify it manually via gdb, with something
like:
for i in a b c; do
git init --bare $i
blob=$(echo $i | git -C $i hash-object -w --stdin)
done
echo "../../b/objects" >a/objects/info/alternates
echo "../../c/objects" >>a/objects/info/alternates
echo "../../c/objects" >b/objects/info/alternates
gdb --args git cat-file -e $blob
After prepare_alt_odb() runs, we have only a single copy of
"/path/to/c/objects/" in the alt_odb list.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_file.c | 7 +------
t/t5613-info-alternate.sh | 4 ++--
2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index b9c1fa3..9a79c19 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -343,12 +343,7 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,
const char *entry = entries.items[i].string;
if (entry[0] == '\0' || entry[0] == '#')
continue;
- if (!is_absolute_path(entry) && depth) {
- error("%s: ignoring relative alternate object store %s",
- relative_base, entry);
- } else {
- link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);
- }
+ link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);
}
string_list_clear(&entries, 0);
free(alt_copy);
diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh
index 9cd2626..b429707 100755
--- a/t/t5613-info-alternate.sh
+++ b/t/t5613-info-alternate.sh
@@ -102,9 +102,9 @@ test_valid_repo'
cd "$base_dir"
test_expect_success \
- 'that relative alternate is only possible for current dir' '
+ 'that relative alternate is recursive' '
cd D &&
- ! (test_valid_repo)
+ test_valid_repo
'
cd "$base_dir"
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 1/6] check_connected: accept an env argument
From: Jeff King @ 2016-09-30 19:35 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net>
This lets callers influence the environment seen by
rev-list, which will be useful when we start providing
quarantined objects.
Signed-off-by: Jeff King <peff@peff.net>
---
connected.c | 1 +
connected.h | 5 +++++
2 files changed, 6 insertions(+)
diff --git a/connected.c b/connected.c
index 8e3e4b1..136c2ac 100644
--- a/connected.c
+++ b/connected.c
@@ -63,6 +63,7 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,
_("Checking connectivity"));
rev_list.git_cmd = 1;
+ rev_list.env = opt->env;
rev_list.in = -1;
rev_list.no_stdout = 1;
if (opt->err_fd)
diff --git a/connected.h b/connected.h
index afa48cc..4ca325f 100644
--- a/connected.h
+++ b/connected.h
@@ -33,6 +33,11 @@ struct check_connected_options {
/* If non-zero, show progress as we traverse the objects. */
int progress;
+
+ /*
+ * Insert these variables into the environment of the child process.
+ */
+ const char **env;
};
#define CHECK_CONNECTED_INIT { 0 }
--
2.10.0.618.g82cc264
^ permalink raw reply related
* [PATCH 0/6] receive-pack: quarantine pushed objects
From: Jeff King @ 2016-09-30 19:35 UTC (permalink / raw)
To: git; +Cc: David Turner
I've mentioned before on the list that GitHub "quarantines" objects
while the pre-receive hook runs. Here are the patches to implement
that.
The basic problem is that as-is, index-pack admits pushed objects into
the main object database immediately, before the pre-receive hook runs.
It _has_ to, since the hook needs to be able to actually look at the
objects. However, this means that if the pre-receive hook rejects the
push, we still end up with the objects in the repository. We can't just
delete them as temporary files, because we don't know what other
processes might have started referencing them.
The solution here is to push into a "quarantine" directory that is
accessible only to pre-receive, check_connected(), etc, and only
move the objects into the main object database after we've finished
those basic checks.
One of the things we use it for at GitHub is object-size policy, which
we implement via a pre-receive hook (sort of; see below). This scheme
has been in use for about 2 years, though I did do a fair bit of
tweaking to make it ready for upstream (squashing bugfixes and merges
from upstream that came later, along with polishing a few rough edges I
saw while doing so). So I may have introduced new bugs. :)
The patches are:
[1/6]: check_connected: accept an env argument
[2/6]: sha1_file: always allow relative paths to alternates
These two are preparatory.
[3/6]: tmp-objdir: introduce API for temporary object directories
[4/6]: receive-pack: quarantine objects until pre-receive accepts
This is the interesting part.
[5/6]: tmp-objdir: put quarantine information in the environment
[6/6]: tmp-objdir: do not migrate files starting with '.'
These are two changes that I ended up doing later to support another
series. They're not strictly necessary here, but I think they're
worth including now, as they change the visible behavior in minor
ways. It seems like a good idea to start with what I think should be
the final behavior.
The other series is basically an optimization for the object-size
policy. Without it, you are stuck walking the graph again in the
pre-receive hook to find the new objects and check their sizes.
But index-pack can do that for you very cheaply; it has the size of
each object already. But it _doesn't_ produce nice error messages;
it has no idea at what path the objects are found, and it doesn't
know what kind of advice it should give the user.
So what we can do is ask index-pack to make a note of any objects
larger than N bytes, and write their sha1 and size into a file in
the quarantine path. Then the pre-receive hook can look in that log
and generate any nice message it wants. In the common case, the log
is empty, and it does not have to do any work at all.
These two patches set that up by letting index-pack and pre-receive
know that quarantine path and use it to store arbitrary files that
_don't_ get migrated to the main object database (i.e., the log file
mentioned above).
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 0/2] place cherry pick line below commit title
From: Junio C Hamano @ 2016-09-30 19:34 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git, Christian Couder
In-Reply-To: <11e41a94-df8c-494a-584b-e2bc8da2de3a@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>> I vaguely recall that there were some discussion on the definition
>> of "what's a trailer line" with folks from the kernel land, perhaps
>> while discussing the interpret-trailers topic. IIRC, when somebody
>> passes an improved version along, the resulting message's trailer
>> block may look like this:
>>
>> Signed-off-by: Original Author <original@author.xz>
>> [fixed typo in the variable names]
>> Signed-off-by: Somebhody Else <somebody@else.xz>
>>
>> and an obvious "wish" of theirs was to treat not just RFC2822-like
>> "a line that begins with token followed by a colon" but also these
>> short comments as part of the trailer block. Your original wish in
>> [*1*] is to also treat "a line that begin with a whitespace that
>> follows a line that begins with token followed by a colon" as part
>> of the trailer block and I personally think that is a reasonable
>> thing to wish for, too.
>
> If we allowed arbitrary lines in the trailer block, this would solve
> my original problem, yes.
OK.
> Looking at that, it seems that sequencer.c started interpreting the
> last paragraph of the commit message as a footer and adding an
> exception for "cherry picked from" in commit b971e04 ("sequencer.c:
> always separate "(cherry picked from" from commit body",
> 2013-02-12). So the interpretations of sequencer.c and
> interpret-trailers were already divergent, but I should have probably
> at least discussed that.
It is not too late to discuss it. I still think it is a good longer
term plan to try to unify the definition of what a trailer block is
and the implementation of the code that determines the boundary
between the log message proper and the trailer block and that allows
us to manipulate the trailer block, that currently is scattered
across multiple places into one. Historically, "commit -s" had one
(because it needed to decide if it needs to see if the last sign-off
is already the one it is adding, and to decide if a blank line is
needed before the sing-off being added), "am -s" had another, and
"cherry-pick" probably had one, too. "interpret-trailers" was, at
least originally, envisioned as an effort to develop a unified
machinery that can be called from these codepaths, and to aid the
development and encourage its use, it also had its own end-user
facing command. Your interest in the "trailer" topic may be a good
trigger for us to further that original vision.
> As for a reason:
>
> 1) I do not have a specific reason for placing it in that exact
> position, but I would like to be able to place the "cherry picked
> from" line without affecting the last paragraph (specifically, without
> making the "cherry picked from" line the only line in the last
> paragraph).
> ...
> 1a) (Avoiding the footer might also be a good way of more clearly
> defining what the footer is. For example, currently, "cherry picked
> from" is treated as a special case in sequencer.c but not in
> trailer.c, as far as I can tell. If we consistently avoided the
> footer, we wouldn't need such a special case anywhere.)
That is one of the numerous shortcomings of the "interpret-trailers"
that is still not finished, I would say.
> 2) The Linux kernel's repository has some "commit ... upstream." lines
> in this position (below the commit title) - for example, in commit
> dacc0987fd2e.
"A group of people seem to prefer it there" does not lead to
"therefore let's move it there for everybody". It does open a
possibility that we may want to add a new option to put it there,
but does not justify changing what existing "-x" option does.
^ permalink raw reply
* Re: [PATCH] diff_unique_abbrev(): document its assumtion and limitation
From: Junio C Hamano @ 2016-09-30 19:19 UTC (permalink / raw)
To: Jeff King; +Cc: git, Linus Torvalds
In-Reply-To: <20160930180957.xj4jqoslbtevhqpb@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... Now that function _would_
> want to be updated as a result of the other conversation (it would need
> to do something sensible with "-1", like turning it into "7", or
> whatever else is deemed reasonable outside of a repository).
>
> Anyway. I just wonder if you want to give it a better name while you are
> at it.
I'd say the patch to introduce the new function that makes the old
name potentially confusing is a good one to do the rename. Until
then I do not think there is no need to rename the existing one ;-)
Related tangent about "like turning it into", I am thinking adding
something like this as a preparatory step to Linus's auto-sizing
serires. That way, we do not have to spell "7"
Having to spell FALLBACK_DEFAULT_ABBREV over and over again would be
more irritating than having to spell "7" often, but I think it would
be a sign of a deeper problem if it turns out we have to repeat this
constant in many places, so an irritatingly long name may serve as a
canary in the coalmine ;-)
-- >8 --
Subject: abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing
We'll be introducing a new way to decide the default abbreviation
length by initialising DEFAULT_ABBREV to -1 to signal the first call
to "find unique abbreviation" codepath to compute a reasonable value
based on the number of objects we have to avoid collisions.
We have long relied on DEFAULT_ABBREV being a positive concrete
value that is used as the abbreviation length when no extra
configuration or command line option has overridden it. Some
codepaths wants to use such a positive concrete default value
even before making their first request to actually trigger the
computation for the auto sized default.
Introduce FALLBACK_DEFAULT_ABBREV and use it to the code that
attempts to align the report from "git fetch". For now, this
macro is also used to initialize the default_abbrev variable,
but the auto-sizing code will use -1 and then use the value of
FALLBACK_DEFAULT_ABBREV as the starting point of auto-sizing.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/fetch.c | 3 +++
cache.h | 3 +++
environment.c | 2 +-
transport.h | 3 +--
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index e4639d8eb1..5d6994d8e7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -16,6 +16,9 @@
#include "connected.h"
#include "argv-array.h"
+#define TRANSPORT_SUMMARY(x) \
+ (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
+
static const char * const builtin_fetch_usage[] = {
N_("git fetch [<options>] [<repository> [<refspec>...]]"),
N_("git fetch [<options>] <group>"),
diff --git a/cache.h b/cache.h
index 4ff196c259..677554c59f 100644
--- a/cache.h
+++ b/cache.h
@@ -1133,6 +1133,9 @@ static inline unsigned int hexval(unsigned char c)
#define MINIMUM_ABBREV minimum_abbrev
#define DEFAULT_ABBREV default_abbrev
+/* used when the code does not know or care what the default abbrev is */
+#define FALLBACK_DEFAULT_ABBREV 7
+
struct object_context {
unsigned char tree[20];
char path[PATH_MAX];
diff --git a/environment.c b/environment.c
index 96160a75a5..c8860f722d 100644
--- a/environment.c
+++ b/environment.c
@@ -16,7 +16,7 @@ int trust_executable_bit = 1;
int trust_ctime = 1;
int check_stat = 1;
int has_symlinks = 1;
-int minimum_abbrev = 4, default_abbrev = 7;
+int minimum_abbrev = 4, default_abbrev = FALLBACK_DEFAULT_ABBREV;
int ignore_case;
int assume_unchanged;
int prefer_symlink_refs;
diff --git a/transport.h b/transport.h
index c68140892c..ea25e42317 100644
--- a/transport.h
+++ b/transport.h
@@ -135,8 +135,7 @@ struct transport {
#define TRANSPORT_PUSH_CERT_IF_ASKED 4096
#define TRANSPORT_PUSH_ATOMIC 8192
-#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
-#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
+#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)
/* Returns a transport suitable for the url */
struct transport *transport_get(struct remote *, const char *);
^ permalink raw reply related
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-30 19:00 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jeff King, Johannes Sixt, Git Mailing List,
Nguyễn Thái Ngọc Duy
In-Reply-To: <CA+55aFyDqYCBCvw0MjZ8fNhaVbRSjsSXNDH--unYkoeJNwVcVg@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> Considering that TRANSPORT_SUMMARY and TRANSPORT_SUMMARY_WIDTH are
> both used in exactly one place each, I'd suggest getting rid of that
> crazy macro, and just expanding it in those places to avoid these
> kinds of crazy "hiding variables inside complex defines thning".
>
> And maybe just deciding to hardcode TRANSPORT_SUMMARY_WIDTH to 17
> (which was it's original default value and presumably is what the test
> is effectively hardcoded for too), and avoiding that complexity
> entirely.
For all fairness, when the WIDTH thing was introduced, there were
two places that needed reference it at f1863d0d16 ("refactor
duplicated code in builtin-send-pack.c and transport.c",
2010-02-16). But that is no longer the case, and it makes sense to
hardcode it as 17 (or something derived from a symbolic constant
that gives the new "default to default").
What TRANSPORT_SUMMARY() does is even more crazy and it really
shouldn't be exposed as a public interface. Let's move it to its
single calling place.
^ permalink raw reply
* Re: [PATCH v8 11/11] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-09-30 18:56 UTC (permalink / raw)
To: Jakub Narębski
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <91589466-439e-7200-7256-b9288beae685@gmail.com>
> On 27 Sep 2016, at 00:41, Jakub Narębski <jnareb@gmail.com> wrote:
>
> Part first of the review of 11/11.
>
> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com pisze:
>> From: Lars Schneider <larsxschneider@gmail.com>
>>
>> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
>> index 7aff940..946dcad 100644
>> --- a/Documentation/gitattributes.txt
>> +++ b/Documentation/gitattributes.txt
>> @@ -293,7 +293,13 @@ checkout, when the `smudge` command is specified, the command is
>> fed the blob object from its standard input, and its standard
>> output is used to update the worktree file. Similarly, the
>> `clean` command is used to convert the contents of worktree file
>> -upon checkin.
>> +upon checkin. By default these commands process only a single
>> +blob and terminate. If a long running `process` filter is used
> ^^^^
>
> Should we use this terminology here? I have not read the preceding
> part of documentation, so I don't know if it talks about "blobs" or
> if it uses "files" and/or "file contents".
I used that because it was used in the paragraph above already.
>> +Long Running Filter Process
>> +^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +
>> +If the filter command (a string value) is defined via
>> +`filter.<driver>.process` then Git can process all blobs with a
>> +single filter invocation for the entire life of a single Git
>> +command. This is achieved by using a packet format (pkt-line,
>> +see technical/protocol-common.txt) based protocol over standard
>> +input and standard output as follows. All packets are considered
>> +text and therefore are terminated by an LF. Exceptions are the
>> +"*CONTENT" packets and the flush packet.
>
> I guess that reasoning here is that all but CONTENT packets are
> metadata, and thus to aid debuggability of the protocol are "text",
> as considered by pkt-line.
>
> Perhaps a bit more readable would be the following (but current is
> just fine; I am nitpicking):
>
> All packets, except for the "{star}CONTENT" packets and the "0000"
> flush packer, are considered text and therefore are terminated by
> a LF.
OK, I use that!
> I think it might be a good idea to describe what flush packet is
> somewhere in this document; on the other hand referring (especially
> if hyperlinked) to pkt-line technical documentation might be good
> enough / better. I'm unsure, but I tend on the side that referring
> to technical documentation is better.
I have this line in the first paragraph of the Long Running Filter process:
"packet format (pkt-line, see technical/protocol-common.txt) based protocol"
>
>> +to read a welcome response message ("git-filter-server") and exactly
>> +one protocol version number from the previously sent list. All further
>
> I guess that is to provide forward-compatibility, isn't it? Also,
> "Git expects..." probably means filter process MUST send, in the
> RFC2119 (https://tools.ietf.org/html/rfc2119) meaning.
True. I feel "expects" reads better but I am happy to change it if
you feel strong about it.
>> +
>> +After the version negotiation Git sends a list of supported capabilities
>> +and a flush packet.
>
> Is it that Git SHOULD send list of ALL supported capabilities, or is
> it that Git SHOULD NOT send capabilities it does not support, and that
> it MAY send only those capabilities it needs (so for example if command
> uses only `smudge`, it may not send `clean`, so that filter driver doesn't
> need to initialize data it would not need).
"After the version negotiation Git sends a list of all capabilities that
it supports and a flush packet."
Better?
> I wonder why it is "<capability>=true", and not "capability=<capability>".
> Is there a case where we would want to send "<capability>=false". Or
> is it to allow configurable / value based capabilities? Isn't it going
> a bit too far: is there even a hind of an idea for parametrize-able
> capability? YAGNI is a thing...
Peff suggested that format and I think it is OK:
http://public-inbox.org/git/20160803224619.bwtbvmslhuicx2qi@sigill.intra.peff.net/
> A few new capabilities that we might want to support in the near future
> is "size", "stream", which are options describing how to communicate,
> and "cleanFromFile", "smudgeToFile", which are new types of operations...
> but neither needs any parameter.
>
> I guess that adding new capabilities doesn't require having to come up
> with the new version of the protocol, isn't it.
Correct.
>> +packet: git< git-filter-server
>> +packet: git< version=2
>> +packet: git> clean=true
>> +packet: git> smudge=true
>> +packet: git> not-yet-invented=true
>
> Hmmm... should we hint at the use of kebab-case versus snake_case
> or camelCase for new capabilities?
I personally prefer kebab-case but I think that is a discussion for
future contributions ;-)
>> +------------------------
>> +packet: git> command=smudge
>> +packet: git> pathname=path/testfile.dat
>> +packet: git> 0000
>> +packet: git> CONTENT
>> +packet: git> 0000
>> +------------------------
>
> I think it is important to mention that (at least with current
> `filter.<driver>.process` implementation, that is absent future
> "stream" capability / option) the filter process needs to read
> *whole contents* at once, *before* writing anything. Otherwise
> it can lead to deadlock.
>
> This is especially important in that it is different (!) from the
> current behavior of `clean` and `smudge` filters, which can
> stream their response because Git invokes them async.
I added this:
" Please note, that the filter
must not send any response before it received the content and the
final flush packet. "
>> +
>> +If the filter experiences an error during processing, then it can
>> +send the status "error" after the content was (partially or
>> +completely) sent. Depending on the `filter.<driver>.required` flag
>> +Git will interpret that as error but it will not stop or restart the
>> +filter process.
>> +------------------------
>> +packet: git< status=success
>> +packet: git< 0000
>> +packet: git< HALF_WRITTEN_ERRONEOUS_CONTENT
>> +packet: git< 0000
>> +packet: git< status=error
>> +packet: git< 0000
>> +------------------------
>
> Good. A question is if the filter process can send "status=abort"
> after partial contents, or does it need to wait for the next command?
I added:
"expected to respond with an "abort" status at any point in
the protocol."
>> +
>> +After the filter has processed a blob it is expected to wait for
>> +the next "key=value" list containing a command. Git will close
>> +the command pipe on exit. The filter is expected to detect EOF
>> +and exit gracefully on its own.
>
> Good to have it documented.
>
> Anyway, as it is Git command that spawns the filter driver process,
> assuming that the filter process doesn't daemonize itself, wouldn't
> the operating system reap it after its parent process, that is the
> git command it invoked, dies? So detecting EOF is good, but not
> strictly necessary for simple filter that do not need to free
> its resources, or can leave freeing resources to the operating
> system? But I may be wrong here.
The filter process runs independent of Git.
>> +
>> +
>> Interaction between checkin/checkout attributes
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> diff --git a/contrib/long-running-filter/example.pl b/contrib/long-running-filter/example.pl
>> new file mode 100755
>> index 0000000..c13a631
>> --- /dev/null
>> +++ b/contrib/long-running-filter/example.pl
>
> To repeat myself, I think it would serve better as a separate patch.
OK
>> + die "invalid packet size '$bytes_read' field";
>
> This would read "invalid packet size '000' field", for example.
> Perhaps the following would be (slightly) better:
>
> + die "invalid packet size field: '$bytes_read'";
OK
>> + }
>> + elsif ( $pkt_size > 4 ) {
>
> Isn't a packet of $pkt_size == 4 a valid packet, a keep-alive
> one? Or is it forbidden?
"Implementations SHOULD NOT send an empty pkt-line ("0004")."
Source: Documentation/technical/protocol-common.txt
>> + die "invalid packet ($content_size expected; $bytes_read read)";
>
> This error message would read "invalid packet (12 expected; 10 read)";
> I think it would be better to rephrase it as
>
> + die "invalid packet ($content_size bytes expected; $bytes_read bytes read)";
OK
>> + die "invalid packet size";
>
> I'm not sure if it is worth it (especially for the demo script),
> but perhaps we could show what this invalid size was?
>
> + die "invalid packet size value '$pkt_size'";
OK
>> +sub packet_txt_read {
>> + my ( $res, $buf ) = packet_bin_read();
>> + unless ( $buf =~ /\n$/ ) {
>
> Wouldn't
>
> + unless ( $buf =~ s/\n$// ) {
>
> or (less so)
>
> + unless ( $buf =~ s/\n$\z// ) {
>
> be more idiomatic (and not require use of 'substr')? Remember,
> the s/// substitution quote-like operator returns number of
> substitutions in the scalar context.
OK.
>> + die "A non-binary line SHOULD BE terminated by an LF.";
>
> This is SHOULD be, not MUST be, so perhaps 'warn' would be enough.
> Not that Git should send us such line.
Actually it MUST per protocol definition. I'll change it to MUST.
>> + my ($packet) = @_;
>
> This is equivalent to
>
> + my $packet = shift;
>
> which, I think, is more common for single-parameter subroutines.
>
> Also, this is $data (or $buf), not $packet.
OK
> Perhaps some comment that main begins here?
>
>> +( packet_txt_read() eq ( 0, "git-filter-client" ) ) || die "bad initialize";
>> +( packet_txt_read() eq ( 0, "version=2" ) ) || die "bad version";
>> +( packet_bin_read() eq ( 1, "" ) ) || die "bad version end";
>
> Actually, it is overly strict. It should not fail if there
> are other "version=3", "version=4" etc. lines.
True, but I think for an example this is OK. I'll add a note
to the file header.
>> +
>> +while (1) {
>> + my ($command) = packet_txt_read() =~ /^command=([^=]+)$/;
>> + my ($pathname) = packet_txt_read() =~ /^pathname=([^=]+)$/;
>
> Do we require this order? If it is, is that explained in the
> documentation?
Git sends that order right now but the filter should not rely
on that order.
>> + packet_flush(); # empty list!
>
> This is less "empty list!", and more keeping "status=success" unchanged.
OK
OK means, I agree and I added your suggestion to v9.
Thanks a lot for your review and the comments!
Cheers,
Lars
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-30 18:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Sixt, Git Mailing List
In-Reply-To: <xmqqmvip9qo7.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 30, 2016 at 11:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> There is another instance buried deep in an obscure macro. A
> minimum fix may look like this, but I really hope somebody else
> finds a better approach.
Heh. Yeah, that's just ugly. I assume this is why the odd git fetch
pretty-printing test was off by one column..
Considering that TRANSPORT_SUMMARY and TRANSPORT_SUMMARY_WIDTH are
both used in exactly one place each, I'd suggest getting rid of that
crazy macro, and just expanding it in those places to avoid these
kinds of crazy "hiding variables inside complex defines thning".
And maybe just deciding to hardcode TRANSPORT_SUMMARY_WIDTH to 17
(which was it's original default value and presumably is what the test
is effectively hardcoded for too), and avoiding that complexity
entirely.
Linus
^ 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