* Re: [PATCH 2/4] t13xx: do not assume system config is empty
From: Jeff King @ 2016-09-29 19:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, torvalds
In-Reply-To: <xmqq60pefrvc.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 12:06:15PM -0700, Junio C Hamano wrote:
> I think it deserves a separate patch and the result is more
> understandable. I've queued this for now (on top of a revised 1/4
> that uses GIT_CONFIG_SYSTEM_PATH instead).
Thanks, makes sense (and I like the new variable name better, by the
way).
> -- >8 --
> From: Jeff King <peff@peff.net>
> Date: Thu, 29 Sep 2016 11:29:10 -0700
> Subject: [PATCH] t1300: check also system-wide configuration file in
> --show-origin tests
>
> Because we used to run our tests with GIT_CONFIG_NOSYSTEM, these did
> not test that the system-wide configuration file is also read and
> shown as one of the origins. Create a custom/fake system-wide
> configuration file and make sure it appears in the output, using the
> newly introduced GIT_CONFIG_SYSTEM_PATH mechanism.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Good description.
Signed-off-by: Jeff King <peff@peff.net>
of course.
> @@ -1304,6 +1315,7 @@ test_expect_success '--show-origin with --get-regexp' '
> file:$HOME/.gitconfig user.global true
> file:.git/config user.local true
> EOF
> + GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
> git config --show-origin --get-regexp "user\.[g|l].*" >output &&
> test_cmp expect output
> '
This is one is trying to do a multi-file lookup, but we couldn't look in
the system config before. But to naturally extend it, it ought to look
like this on top:
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index d2476a8..4dd5ce3 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1310,11 +1310,12 @@ test_expect_success '--show-origin with single file' '
test_expect_success '--show-origin with --get-regexp' '
cat >expect <<-EOF &&
+ file:$HOME/etc-gitconfig user.system true
file:$HOME/.gitconfig user.global true
file:.git/config user.local true
EOF
GIT_ETC_GITCONFIG=$HOME/etc-gitconfig \
- git config --show-origin --get-regexp "user\.[g|l].*" >output &&
+ git config --show-origin --get-regexp "user\.[g|l|s].*" >output &&
test_cmp expect output
'
> @@ -1312,6 +1324,7 @@ test_expect_success '--show-origin getting a single key' '
> cat >expect <<-\EOF &&
> file:.git/config local
> EOF
> + GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
> git config --show-origin user.override >output &&
> test_cmp expect output
> '
And I was tempted to say this one should not need to care, but I guess
it is testing that we correctly read the override from the local config
over the global one. So likewise, it is good to check that we also
override the system config (it does not effect the "expect" output, but
that does not mean it is not enhancing the test).
-Peff
^ permalink raw reply related
* [RFC/PATCH 1/2] sequencer: refactor message and origin appending
From: Jonathan Tan @ 2016-09-29 19:21 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1475176070.git.jonathantanmy@google.com>
Move the appending of the commit message and origin information into its
own function, in preparation for a subsequent patch.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
sequencer.c | 46 ++++++++++++++++++++++++++++------------------
1 file changed, 28 insertions(+), 18 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 3804fa9..b29c9ca 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -443,6 +443,33 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
return 1;
}
+/*
+ * Appends the commit log message, including the cherry picked notification if
+ * record_origin is nonzero.
+ */
+static void append_message(struct strbuf *msgbuf,
+ const struct commit_message *msg,
+ int record_origin,
+ const struct commit *commit)
+{
+ /*
+ * Append the commit log message to msgbuf; it starts
+ * after the tree, parent, author, committer
+ * information followed by "\n\n".
+ */
+ const char *p = strstr(msg->message, "\n\n");
+ if (p)
+ strbuf_addstr(msgbuf, skip_blank_lines(p + 2));
+
+ if (record_origin) {
+ if (!has_conforming_footer(msgbuf, NULL, 0))
+ strbuf_addch(msgbuf, '\n');
+ strbuf_addstr(msgbuf, cherry_picked_prefix);
+ strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
+ strbuf_addstr(msgbuf, ")\n");
+ }
+}
+
static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
{
unsigned char head[20];
@@ -534,29 +561,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
}
strbuf_addstr(&msgbuf, ".\n");
} else {
- const char *p;
-
base = parent;
base_label = msg.parent_label;
next = commit;
next_label = msg.label;
- /*
- * Append the commit log message to msgbuf; it starts
- * after the tree, parent, author, committer
- * information followed by "\n\n".
- */
- p = strstr(msg.message, "\n\n");
- if (p)
- strbuf_addstr(&msgbuf, skip_blank_lines(p + 2));
-
- if (opts->record_origin) {
- if (!has_conforming_footer(&msgbuf, NULL, 0))
- strbuf_addch(&msgbuf, '\n');
- strbuf_addstr(&msgbuf, cherry_picked_prefix);
- strbuf_addstr(&msgbuf, oid_to_hex(&commit->object.oid));
- strbuf_addstr(&msgbuf, ")\n");
- }
+ append_message(&msgbuf, &msg, opts->record_origin, commit);
}
if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* [RFC/PATCH 0/2] place cherry pick line below commit title
From: Jonathan Tan @ 2016-09-29 19:21 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
This is somewhat of a follow-up to my previous e-mail with subject
"[PATCH] sequencer: support folding in rfc2822 footer" [1], in which I
proposed relaxing the definition of a commit message footer to allow
multiple-line field bodies (as described in RFC2822), but its strictness
was deemed deliberate.
Below is a patch set that allows placing the "cherry picked from" line
without taking into account the definition of a commit message footer.
For example, "git cherry-pick -x" (with the appropriate configuration
variable or argument) would, to this commit message:
commit title
This is an explanatory paragraph.
Footer: foo
place the "(cherry picked from ...)" line below "commit title".
Would this be better?
[1] <1472846322-5592-1-git-send-email-jonathantanmy@google.com>
Jonathan Tan (2):
sequencer: refactor message and origin appending
sequencer: allow origin line below commit title
Documentation/config.txt | 4 +++
Documentation/git-cherry-pick.txt | 15 ++++++++-
builtin/revert.c | 38 ++++++++++++++++++++-
sequencer.c | 69 +++++++++++++++++++++++++++++----------
sequencer.h | 7 ++++
t/t3511-cherry-pick-x.sh | 59 +++++++++++++++++++++++++++++++++
6 files changed, 172 insertions(+), 20 deletions(-)
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply
* [RFC/PATCH 2/2] sequencer: allow origin line below commit title
From: Jonathan Tan @ 2016-09-29 19:21 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
In-Reply-To: <cover.1475176070.git.jonathantanmy@google.com>
When git cherry-pick -x is invoked, a "(cherry picked from commit ...)"
line is appended to the footer of a commit message that Git interprets
to contain a footer; otherwise it is appended at the end as a new
paragraph, preceded by a blank line. This behavior may appear
inconsistent, especially to users who differ from Git in their
interpretation of what constitutes a footer.
Provide the user, through a configuration option and command-line flag,
the option of placing the "cherry picked" line below the commit title
instead of the current behavior. This allows the "cherry picked" line
to be placed in a consistent manner, independent of the nature of the
footer of the existing commit message.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Documentation/config.txt | 4 +++
Documentation/git-cherry-pick.txt | 15 +++++++++-
builtin/revert.c | 38 ++++++++++++++++++++++++-
sequencer.c | 39 ++++++++++++++++++++------
sequencer.h | 7 +++++
t/t3511-cherry-pick-x.sh | 59 +++++++++++++++++++++++++++++++++++++++
6 files changed, 152 insertions(+), 10 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0bcb679..fb1990f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -945,6 +945,10 @@ browser.<tool>.path::
browse HTML help (see `-w` option in linkgit:git-help[1]) or a
working repository in gitweb (see linkgit:git-instaweb[1]).
+cherrypick.originLineLocation::
+ Default for the `--origin-line-location` option in git-cherry-pick.
+ Defaults to `bottom`.
+
clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n. Defaults to true.
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index d35d771..5a8359f 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -58,7 +58,7 @@ OPTIONS
message prior to committing.
-x::
- When recording the commit, append a line that says
+ When recording the commit, add a line that says
"(cherry picked from commit ...)" to the original commit
message in order to indicate which commit this change was
cherry-picked from. This is done only for cherry
@@ -71,6 +71,14 @@ OPTIONS
development branch), adding this information can be
useful.
+--origin-line-location::
+ Where to put the "(cherry picked from commit ...)" line when requested
+ with the `-x` option. May be `top`, meaning at the top of the commit
+ message body, immediately below the commit title (see the DISCUSSION
+ section of linkgit:git-commit[1]), or `bottom`, meaning at the end of
+ the commit message. The default is controlled by the
+ `cherrypick.originLineLocation` configuration variable.
+
-r::
It used to be that the command defaulted to do `-x`
described above, and `-r` was to disable it. Now the
@@ -224,6 +232,11 @@ the working tree.
spending extra time to avoid mistakes based on incorrectly matching
context lines.
+CONFIGURATION
+-------------
+cherrypick.originLineLocation::
+ Default for the `--origin-line-location` option. Defaults to `bottom`.
+
SEE ALSO
--------
linkgit:git-revert[1]
diff --git a/builtin/revert.c b/builtin/revert.c
index 4e69380..a5459a0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -71,11 +71,25 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)
die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
}
+static int set_origin_line(enum origin_line *line, const char *str)
+{
+ if (!strcmp(str, "bottom")) {
+ *line = ORIGIN_LINE_BOTTOM;
+ return 1;
+ }
+ if (!strcmp(str, "top")) {
+ *line = ORIGIN_LINE_TOP;
+ return 1;
+ }
+ return 0;
+}
+
static void parse_args(int argc, const char **argv, struct replay_opts *opts)
{
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
int cmd = 0;
+ const char *origin_str = NULL;
struct option base_options[] = {
OPT_CMDMODE(0, "quit", &cmd, N_("end revert or cherry-pick sequence"), 'q'),
OPT_CMDMODE(0, "continue", &cmd, N_("resume revert or cherry-pick sequence"), 'c'),
@@ -98,6 +112,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
if (opts->action == REPLAY_PICK) {
struct option cp_extra[] = {
OPT_BOOL('x', NULL, &opts->record_origin, N_("append commit name")),
+ OPT_STRING(0, "origin-line-location", &origin_str, N_("origin-line-location"), N_("location of appended commit name")),
OPT_BOOL(0, "ff", &opts->allow_ff, N_("allow fast-forward")),
OPT_BOOL(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
OPT_BOOL(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
@@ -125,6 +140,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
else
opts->subcommand = REPLAY_NONE;
+ /* Set the origin line location */
+ if (origin_str)
+ if (!set_origin_line(&opts->origin_line, origin_str))
+ die(_("%s: --origin-line-location must be top or bottom"),
+ me);
+
/* Check for incompatible command line arguments */
if (opts->subcommand != REPLAY_NONE) {
char *this_operation;
@@ -176,6 +197,21 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
usage_with_options(usage_str, options);
}
+static int git_cherry_pick_config(const char *var, const char *value,
+ void *opts_)
+{
+ struct replay_opts *opts = opts_;
+
+ if (!strcmp(var, "cherrypick.originlinelocation")) {
+ if (!value)
+ return config_error_nonbool(var);
+ set_origin_line(&opts->origin_line, value);
+ return 0;
+ }
+
+ return git_default_config(var, value, opts_);
+}
+
int cmd_revert(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts;
@@ -200,7 +236,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
memset(&opts, 0, sizeof(opts));
opts.action = REPLAY_PICK;
- git_config(git_default_config, NULL);
+ git_config(git_cherry_pick_config, &opts);
parse_args(argc, argv, &opts);
res = sequencer_pick_revisions(&opts);
if (res < 0)
diff --git a/sequencer.c b/sequencer.c
index b29c9ca..ef9e5bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -450,23 +450,45 @@ static int allow_empty(struct replay_opts *opts, struct commit *commit)
static void append_message(struct strbuf *msgbuf,
const struct commit_message *msg,
int record_origin,
+ enum origin_line origin_line,
const struct commit *commit)
{
/*
- * Append the commit log message to msgbuf; it starts
+ * The commit log message starts
* after the tree, parent, author, committer
* information followed by "\n\n".
*/
const char *p = strstr(msg->message, "\n\n");
- if (p)
- strbuf_addstr(msgbuf, skip_blank_lines(p + 2));
+ p = skip_blank_lines(p + 2);
+ if (!record_origin) {
+ strbuf_addstr(msgbuf, p);
+ return;
+ }
- if (record_origin) {
+ switch (origin_line) {
+ case ORIGIN_LINE_TOP:
+ /* First, add only the subject. */
+ p = format_subject(msgbuf, p, "\n");
+ strbuf_addstr(msgbuf, "\n\n");
+ break;
+ case ORIGIN_LINE_BOTTOM:
+ strbuf_addstr(msgbuf, p);
if (!has_conforming_footer(msgbuf, NULL, 0))
strbuf_addch(msgbuf, '\n');
- strbuf_addstr(msgbuf, cherry_picked_prefix);
- strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
- strbuf_addstr(msgbuf, ")\n");
+ break;
+ }
+
+ strbuf_addstr(msgbuf, cherry_picked_prefix);
+ strbuf_addstr(msgbuf, oid_to_hex(&commit->object.oid));
+ strbuf_addstr(msgbuf, ")\n");
+
+ if (origin_line == ORIGIN_LINE_TOP) {
+ /* Add the rest of the commit message. */
+ p = skip_blank_lines(p);
+ if (*p) {
+ strbuf_addch(msgbuf, '\n');
+ strbuf_addstr(msgbuf, p);
+ }
}
}
@@ -566,7 +588,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
next = commit;
next_label = msg.label;
- append_message(&msgbuf, &msg, opts->record_origin, commit);
+ append_message(&msgbuf, &msg, opts->record_origin,
+ opts->origin_line, commit);
}
if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
diff --git a/sequencer.h b/sequencer.h
index 5ed5cb1..7cf381c 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -20,6 +20,11 @@ enum replay_subcommand {
REPLAY_ROLLBACK
};
+enum origin_line {
+ ORIGIN_LINE_BOTTOM,
+ ORIGIN_LINE_TOP
+};
+
struct replay_opts {
enum replay_action action;
enum replay_subcommand subcommand;
@@ -46,6 +51,8 @@ struct replay_opts {
/* Only used by REPLAY_NONE */
struct rev_info *revs;
+
+ enum origin_line origin_line;
};
int sequencer_pick_revisions(struct replay_opts *opts);
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..57e3861 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -244,4 +244,63 @@ test_expect_success 'cherry-pick preserves commit message' '
test_cmp expect actual
'
+mesg_one_para="This is a commit message
+in one paragraph"
+
+test_expect_success 'cherry-pick -x (top location) one-paragraph commit message' '
+ pristine_detach initial &&
+ test_config cherrypick.originLineLocation top &&
+ test_commit "$mesg_one_para" foo b mesg-one-para &&
+ git reset --hard initial &&
+ sha1=$(git rev-parse mesg-one-para^0) &&
+ git cherry-pick -x mesg-one-para &&
+ cat <<-EOF >expect &&
+ $mesg_one_para
+
+ (cherry picked from commit $sha1)
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+space=" "
+mesg_multi_para="$mesg_one_para
+$space
+
+$mesg_one_para"
+
+test_expect_success 'cherry-pick -x (top location) multi-paragraph commit message' '
+ pristine_detach initial &&
+ test_config cherrypick.originLineLocation top &&
+ test_commit "$mesg_multi_para" foo b mesg-multi-para &&
+ git reset --hard initial &&
+ sha1=$(git rev-parse mesg-multi-para^0) &&
+ git cherry-pick -x mesg-multi-para &&
+ cat <<-EOF >expect &&
+ $mesg_one_para
+
+ (cherry picked from commit $sha1)
+
+ $mesg_one_para
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -x location argument overrides config' '
+ test_config cherrypick.originLineLocation top &&
+ git reset --hard initial &&
+ sha1=$(git rev-parse mesg-multi-para^0) &&
+ git cherry-pick -x --origin-line-location=bottom mesg-multi-para &&
+ cat <<-EOF >expect &&
+ $mesg_one_para
+
+ $mesg_one_para
+
+ (cherry picked from commit $sha1)
+ EOF
+ git log -1 --pretty=format:%B >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH 2/4] t13xx: do not assume system config is empty
From: Jeff King @ 2016-09-29 19:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, torvalds
In-Reply-To: <xmqqa8eqfsap.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 11:57:02AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> "either" meaning "we do not need to add --local and we do not need
> >> GIT_CONFIG_NOSYSTEM"?
> >
> > Yes. I didn't test it with your core.abbrev patch 4/4, but I _didn't_
> > have to touch their expected output after pointing them at a non-empty
> > etc-gitconfig file in the trash directory. Which implies to me they
> > don't care either way (which makes sense; they are asking for a specific
> > key which is supposed to be found in one of the other files).
>
> There is a bit of problem here, though.
>
> * If we make t1300 point at its own system-wide config, it will be
> in control of its contents, so "find this key" will find only it
> wants to find (or we found a regression).
>
> * But then if it ever does something that depends on the default
> value of core.abbrev (or whatever we'd tweak in response to the
> next suggestion by Linus ;-), we cannot really allow it to do
> so. We'd want t/gitconfig-for-test to be the single place that
> we can tweak these things, but we'll have to know t1300 uses its
> own and need to make the same change there, too.
Right, but I think that's fine. Tests that care deeply about the
contents of etc-gitconfig are unlikely to care about core.abbrev. And in
the off chance that they do, then the worst case is...they get updated
to handle core.abbrev (either passing a command line option, or just
putting core.abbrev in their test file).
I just don't see it being a problem. Adding core.abbrev for the whole
test suite is just about not having a big flag day where we change all
the tests. Changing one or two tests (and again, I'd be surprised if we
even have to do that) doesn't seem like a big deal.
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Jeff King @ 2016-09-29 19:16 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Johannes Sixt, Git Mailing List
In-Reply-To: <CA+55aFwbCNiF0nDppZ5SuRcZwc9kNvKYzgyd_bR8Ut8XRW_p4Q@mail.gmail.com>
On Thu, Sep 29, 2016 at 11:55:46AM -0700, Linus Torvalds wrote:
> I think the patch can speak for itself, but the basic core is this
> section in get_short_sha1():
>
> + if (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) {
> + unsigned int expect_collision = 1 << (len * 2);
> + if (ds.nrobjects > expect_collision)
> + return SHORT_NAME_AMBIGUOUS;
> + }
Hmm. So at length 7, we expect collisions at 2^14, which is 16384. That
seems really low. I mean, by the birthday paradox that's where expect
a 50% chance of a collision. But that's a single collision. We
definitely don't expect them to be common at that size.
So I suspect this could be a bit looser. The real number we care about
is probably something like "there is probability 'p' of a collision when
we add a new object", but I'm not sure what that 'p' would be. Or
perhaps "we accept collisions in 'n' percent of objects". But again, I
don't know that 'n'.
I dunno. I suppose being overly conservative with this number leaves
room for growth. Repositories generally get bigger, not smaller. :)
> What do you think? It's actually a fairly simple patch and I really do
> think it makes sense and it seems to just DTRT automatically.
I like the general idea.
As far as the implementation, I was surprised to see it touch
get_short_sha1() at all. That's, after all, for lookups, and we would
never want to require more characters on the reading side.
I see you worked around it with a flag so that this behavior only kicks
in when called via find_unique_abbrev(). But if you look at the caller:
> @@ -458,14 +472,19 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
> int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
> {
> int status, exists;
> + int flags = GET_SHA1_QUIETLY;
>
> + if (len < 0) {
> + flags |= GET_SHA1_AUTOMATIC;
> + len = 7;
> + }
> sha1_to_hex_r(hex, sha1);
> if (len == 40 || !len)
> return 40;
> exists = has_sha1_file(sha1);
> while (len < 40) {
> unsigned char sha1_ret[20];
> - status = get_short_sha1(hex, len, sha1_ret, GET_SHA1_QUIETLY);
> + status = get_short_sha1(hex, len, sha1_ret, flags);
> if (exists
> ? !status
> : status == SHORT_NAME_NOT_FOUND) {
You can see that we're going to do more work than we would otherwise
need to. Because we start at 7, and ask get_short_sha1() "is this unique
enough?", and looping. But if we _know_ we won't accept any answer
shorter than some N based on the number of objects in the repository,
then we should start at that N.
IOW, something like:
if (len < 0)
len = ceil(log_base_2(repository_object_count()));
here, and then you don't have to touch get_short_sha1() at all.
I suspect you pushed it down into get_short_sha1() because it kind-of
does the repository_object_count() step for "free" as it's looking at
the object anyway. But that step is really not very expensive. And I'd
even say you could just ignore loose objects entirely, and treat them
like a rounding error (the way that duplicate objects in packs are
treated).
That leaves you with just an O(# of packs) loop over a linked list. You
could even just keep a global object count up to date in
add_packed_git(), and then it's O(1).
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-29 19:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List, Jeff King
In-Reply-To: <CA+55aFwbCNiF0nDppZ5SuRcZwc9kNvKYzgyd_bR8Ut8XRW_p4Q@mail.gmail.com>
On Thu, Sep 29, 2016 at 11:55 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> For the kernel, just the *math* right now actually gives 12
> characters. For current git it actually seems to say that 8 is the
> correct number. For small projects, you'll still see 7.
Sorry, the git number is 9, not 8. The reason is that git has roughly
212k objects, and 9 hex digits gets expected collisions at about 256k
objects.
So the logic means that we'll see 7 hex digits for projects with less
than 16k objects, 8 hex digits if there are less than 64k objects, and
9 hex digits for projects like git that currently have fewer than 256k
objects.
But git itself might not be *that* far from going to 10 hex digits
with my patch.
The kernel uses 12 he digits because the collision math says that's
the right thing for a project with between 4M and 16M objects (with
the kernel being at 5M).
So on the whole the patch really does seem to just do the right thing
automatically.
Linus
^ permalink raw reply
* Re: [PATCH 2/4] t13xx: do not assume system config is empty
From: Junio C Hamano @ 2016-09-29 19:06 UTC (permalink / raw)
To: Jeff King; +Cc: git, torvalds
In-Reply-To: <20160929182621.lobihscwl7amtu7s@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Sep 29, 2016 at 11:13:45AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > I think anytime you would use GIT_CONFIG_NOSYSTEM over --local, it is an
>> > indication that the test is trying to check how multiple sources
>> > interact. And the right thing to do for them is to set GIT_ETC_GITCONFIG
>> > to some known quantity. We just couldn't do that before, so we skipped
>> > it. IOW, something like the patch below (on top of yours).
>>
>> OK, that way we can make sure that "multiple sources" operations do
>> look at the system-wide stuff.
>
> Exactly.
I think it deserves a separate patch and the result is more
understandable. I've queued this for now (on top of a revised 1/4
that uses GIT_CONFIG_SYSTEM_PATH instead).
-- >8 --
From: Jeff King <peff@peff.net>
Date: Thu, 29 Sep 2016 11:29:10 -0700
Subject: [PATCH] t1300: check also system-wide configuration file in
--show-origin tests
Because we used to run our tests with GIT_CONFIG_NOSYSTEM, these did
not test that the system-wide configuration file is also read and
shown as one of the origins. Create a custom/fake system-wide
configuration file and make sure it appears in the output, using the
newly introduced GIT_CONFIG_SYSTEM_PATH mechanism.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t1300-repo-config.sh | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 0543b62227bf..aa25577709c5 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1236,6 +1236,11 @@ test_expect_success 'set up --show-origin tests' '
[user]
relative = include
EOF
+ cat >"$HOME"/etc-gitconfig <<-\EOF &&
+ [user]
+ system = true
+ override = system
+ EOF
cat >"$HOME"/.gitconfig <<-EOF &&
[user]
global = true
@@ -1254,6 +1259,8 @@ test_expect_success 'set up --show-origin tests' '
test_expect_success '--show-origin with --list' '
cat >expect <<-EOF &&
+ file:$HOME/etc-gitconfig user.system=true
+ file:$HOME/etc-gitconfig user.override=system
file:$HOME/.gitconfig user.global=true
file:$HOME/.gitconfig user.override=global
file:$HOME/.gitconfig include.path=$INCLUDE_DIR/absolute.include
@@ -1264,13 +1271,16 @@ test_expect_success '--show-origin with --list' '
file:.git/../include/relative.include user.relative=include
command line: user.cmdline=true
EOF
+ GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
git -c user.cmdline=true config --list --show-origin >output &&
test_cmp expect output
'
test_expect_success '--show-origin with --list --null' '
cat >expect <<-EOF &&
- file:$HOME/.gitconfigQuser.global
+ file:$HOME/etc-gitconfigQuser.system
+ trueQfile:$HOME/etc-gitconfigQuser.override
+ systemQfile:$HOME/.gitconfigQuser.global
trueQfile:$HOME/.gitconfigQuser.override
globalQfile:$HOME/.gitconfigQinclude.path
$INCLUDE_DIR/absolute.includeQfile:$INCLUDE_DIR/absolute.includeQuser.absolute
@@ -1281,6 +1291,7 @@ test_expect_success '--show-origin with --list --null' '
includeQcommand line:Quser.cmdline
trueQ
EOF
+ GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
git -c user.cmdline=true config --null --list --show-origin >output.raw &&
nul_to_q <output.raw >output &&
# The here-doc above adds a newline that the --null output would not
@@ -1304,6 +1315,7 @@ test_expect_success '--show-origin with --get-regexp' '
file:$HOME/.gitconfig user.global true
file:.git/config user.local true
EOF
+ GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
git config --show-origin --get-regexp "user\.[g|l].*" >output &&
test_cmp expect output
'
@@ -1312,6 +1324,7 @@ test_expect_success '--show-origin getting a single key' '
cat >expect <<-\EOF &&
file:.git/config local
EOF
+ GIT_CONFIG_SYSTEM_PATH=$HOME/etc-gitconfig \
git config --show-origin user.override >output &&
test_cmp expect output
'
--
2.10.0-589-g5adf4e1
^ permalink raw reply related
* Re: [PATCH 2/4] t13xx: do not assume system config is empty
From: Junio C Hamano @ 2016-09-29 18:57 UTC (permalink / raw)
To: Jeff King; +Cc: git, torvalds
In-Reply-To: <20160929182621.lobihscwl7amtu7s@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> "either" meaning "we do not need to add --local and we do not need
>> GIT_CONFIG_NOSYSTEM"?
>
> Yes. I didn't test it with your core.abbrev patch 4/4, but I _didn't_
> have to touch their expected output after pointing them at a non-empty
> etc-gitconfig file in the trash directory. Which implies to me they
> don't care either way (which makes sense; they are asking for a specific
> key which is supposed to be found in one of the other files).
There is a bit of problem here, though.
* If we make t1300 point at its own system-wide config, it will be
in control of its contents, so "find this key" will find only it
wants to find (or we found a regression).
* But then if it ever does something that depends on the default
value of core.abbrev (or whatever we'd tweak in response to the
next suggestion by Linus ;-), we cannot really allow it to do
so. We'd want t/gitconfig-for-test to be the single place that
we can tweak these things, but we'll have to know t1300 uses its
own and need to make the same change there, too.
So, I dunno.
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-29 18:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List, Jeff King
In-Reply-To: <CA+55aFyYWWpz+9+KKf=9y3vBrEDyy-5h6J3boiitGE7Zb=uL-Q@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]
On Thu, Sep 29, 2016 at 11:37 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I'm playing with an early patch to make the default more dynamic.
> Let's see how well it works in practice, but it looks fairly
> promising. Let me test a bit more and send out an RFC patch..
Ok, this is *very* rough, and it doesn't actuall pass all the tests,
and I didn't even try to look at why. But it passes the trivial
smell-test, and in particular it actually makes mathematical sense...
I think the patch can speak for itself, but the basic core is this
section in get_short_sha1():
+ if (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) {
+ unsigned int expect_collision = 1 << (len * 2);
+ if (ds.nrobjects > expect_collision)
+ return SHORT_NAME_AMBIGUOUS;
+ }
basically, what it says is that we will consider a sha1 ambiguous even
if it was *technically* unique (that's the '!status' part of the test)
if:
- the length was 15 or less
*and*
- the number of objects we have is larger than the expected point
where statistically we should start to expect to get one collision.
That "expect_collision" math is actually very simple: each hex
character adds four bits of range, but since we expect collisions at
the square root of the maximum number of objects, we shift by just two
bits per hex digits instead.
The rest of the patch is a trivial change to just initialize the
default short size to -1, and consider that to mean "enable the
automatic size checking with a minimum of 7". And the trivial code to
estimate the number of objects (which ignores duplicates between packs
etc _entirely_).
For the kernel, just the *math* right now actually gives 12
characters. For current git it actually seems to say that 8 is the
correct number. For small projects, you'll still see 7.
ANYWAY. This patch is on top of Jeff's patches in 'pu' (I think those
are great regardless of this patch!), and as mentioned, it fails some
tests. I suspect that the failures might be due to the abbrev_default
being -1, and some other code finds that surprising now. But as
mentioned, I didn't really even look at it.
What do you think? It's actually a fairly simple patch and I really do
think it makes sense and it seems to just DTRT automatically.
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/plain, Size: 2976 bytes --]
cache.h | 1 +
environment.c | 2 +-
sha1_name.c | 21 ++++++++++++++++++++-
3 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 6e33f2f..d2da6d1 100644
--- a/cache.h
+++ b/cache.h
@@ -1207,6 +1207,7 @@ struct object_context {
#define GET_SHA1_TREEISH 020
#define GET_SHA1_BLOB 040
#define GET_SHA1_FOLLOW_SYMLINKS 0100
+#define GET_SHA1_AUTOMATIC 0200
#define GET_SHA1_ONLY_TO_DIE 04000
#define GET_SHA1_DISAMBIGUATORS \
diff --git a/environment.c b/environment.c
index c1442df..fd6681e 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 = -1;
int ignore_case;
int assume_unchanged;
int prefer_symlink_refs;
diff --git a/sha1_name.c b/sha1_name.c
index 3b647fd..8791ff3 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -15,6 +15,7 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
struct disambiguate_state {
int len; /* length of prefix in hex chars */
+ unsigned int nrobjects;
char hex_pfx[GIT_SHA1_HEXSZ + 1];
unsigned char bin_pfx[GIT_SHA1_RAWSZ];
@@ -118,6 +119,12 @@ static void find_short_object_filename(struct disambiguate_state *ds)
if (strlen(de->d_name) != 38)
continue;
+
+ // We only look at the one subdirectory, and we assume
+ // each subdirectory is roughly similar, so each object
+ // we find probably has 255 other objects in the other
+ // fan-out directories
+ ds->nrobjects += 256;
if (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))
continue;
memcpy(hex + 2, de->d_name, 38);
@@ -151,6 +158,7 @@ static void unique_in_pack(struct packed_git *p,
open_pack_index(p);
num = p->num_objects;
+ ds->nrobjects += num;
last = num;
while (first < last) {
uint32_t mid = (first + last) / 2;
@@ -426,6 +434,12 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
for_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);
}
+ if (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) {
+ unsigned int expect_collision = 1 << (len * 2);
+ if (ds.nrobjects > expect_collision)
+ return SHORT_NAME_AMBIGUOUS;
+ }
+
return status;
}
@@ -458,14 +472,19 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
{
int status, exists;
+ int flags = GET_SHA1_QUIETLY;
+ if (len < 0) {
+ flags |= GET_SHA1_AUTOMATIC;
+ len = 7;
+ }
sha1_to_hex_r(hex, sha1);
if (len == 40 || !len)
return 40;
exists = has_sha1_file(sha1);
while (len < 40) {
unsigned char sha1_ret[20];
- status = get_short_sha1(hex, len, sha1_ret, GET_SHA1_QUIETLY);
+ status = get_short_sha1(hex, len, sha1_ret, flags);
if (exists
? !status
: status == SHORT_NAME_NOT_FOUND) {
^ permalink raw reply related
* Re: [PATCH/RFC] git log --oneline alternative with dates, times and initials
From: Junio C Hamano @ 2016-09-29 18:50 UTC (permalink / raw)
To: Jeff King; +Cc: Kyle J. McKay, Git mailing list
In-Reply-To: <20160929183006.exyaikr4ijiq5tp3@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> But I also buy the argument that contrib/ is simply a hassle. This
> script can live in its own repository somewhere, and handle
> announcements and patches on the list.
I think the output of this script is largely personal preference,
which can be made to a project preference for a project enough of
whose participant so desires.
For example, I would not be surprised if this appeared next to
checkpatch.pl script in the kernel archive. When a project that
uses Git to store its sources finds a need to summarize its log in a
standardized way that is not produced natively by Git, such a
project may add this script to its scripts/ area, just like a
project that wants to have a standard way to help its contributors
to avoid common style errors a lot more than our "diff" (which only
highlights whitespace errors) does may ship checkpatch.pl in it.
So in that sense, while I do not mean to say that the script itself
must become a standalone project that has only one script in it, I
do not think it belongs "our" contrib/, as we do not see a need to
standardize its output as the log summary standard we the Git
project uses on its own history.
On the other hand, your illustration of the needed bits to express
this particular output format used by Kyle's script, when polished,
does fit in our codebase. We are interested in making it possible
for projects and users to do more by using Git with its standard
customization features.
^ permalink raw reply
* Re: [PATCH v5 1/4] git: make super-prefix option
From: Brandon Williams @ 2016-09-29 18:44 UTC (permalink / raw)
To: Jeff King; +Cc: git, sbeller, gitster
In-Reply-To: <20160929183940.vgac7by74gmglaf2@sigill.intra.peff.net>
On 09/29, Jeff King wrote:
> On Wed, Sep 28, 2016 at 02:50:40PM -0700, Brandon Williams wrote:
>
> > Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
> > which can be used to specify a path from above a repository down to its
> > root. The immediate use of this option is by commands which have a
> > --recurse-submodule option in order to give context to submodules about
> > how they were invoked. This option is currently only allowed for
> > builtins which support a super-prefix.
>
> What about non-builtins?
>
> E.g., what should
>
> git --super-prefix=foo bar
>
> do? Should the externals and scripts check the presence of
> GIT_INTERNAL_SUPER_PREFIX and barf if it is set? Most scripts would
> probably notice eventually when calling some other builtin that doesn't
> support SUPER_PREFIX, but it seems hacky to count on that.
>
> There's also the question of 3rd-party programs. If we want to be
> conservative, I think you'd want to just always bail in
> execv_dashed_external() if --super-prefix is in use. That doesn't give
> an option for scripts to say "hey, I support this", but we can perhaps
> worry about loosening later.
>
> -Peff
That makes sense.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v5 1/4] git: make super-prefix option
From: Jeff King @ 2016-09-29 18:39 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller, gitster
In-Reply-To: <1475099443-145608-2-git-send-email-bmwill@google.com>
On Wed, Sep 28, 2016 at 02:50:40PM -0700, Brandon Williams wrote:
> Add a super-prefix environment variable 'GIT_INTERNAL_SUPER_PREFIX'
> which can be used to specify a path from above a repository down to its
> root. The immediate use of this option is by commands which have a
> --recurse-submodule option in order to give context to submodules about
> how they were invoked. This option is currently only allowed for
> builtins which support a super-prefix.
What about non-builtins?
E.g., what should
git --super-prefix=foo bar
do? Should the externals and scripts check the presence of
GIT_INTERNAL_SUPER_PREFIX and barf if it is set? Most scripts would
probably notice eventually when calling some other builtin that doesn't
support SUPER_PREFIX, but it seems hacky to count on that.
There's also the question of 3rd-party programs. If we want to be
conservative, I think you'd want to just always bail in
execv_dashed_external() if --super-prefix is in use. That doesn't give
an option for scripts to say "hey, I support this", but we can perhaps
worry about loosening later.
-Peff
^ permalink raw reply
* Re: [PATCH v8 00/11] Git filter protocol
From: Johannes Sixt @ 2016-09-29 18:38 UTC (permalink / raw)
To: Torsten Bögershausen
Cc: Lars Schneider, Junio C Hamano, git, Jeff King, Stefan Beller,
Jakub Narębski, Martin-Louis Bright, ramsay
In-Reply-To: <7f8ab626-ecdb-70a8-aa19-615c3c84148e@web.de>
Am 29.09.2016 um 20:18 schrieb Torsten Bögershausen:
> I would agree that Git should not wait for the filter.
> But does the test suite need to wait for the filter ?
We have fixed a test case on Windows recently where a process hung
around too long (5babb5bd). So, yes, the test suite has to wait for the
filter.
-- Hannes
^ permalink raw reply
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Linus Torvalds @ 2016-09-29 18:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Git Mailing List, Jeff King
In-Reply-To: <xmqqmviqfuoh.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Yes, "git log --oneline" looks somewhat different and strange for
> me, too ;-)
I'm playing with an early patch to make the default more dynamic.
Let's see how well it works in practice, but it looks fairly
promising. Let me test a bit more and send out an RFC patch..
Linus
^ permalink raw reply
* Re: [PATCH 5/5] log: add --commit-header option
From: Jeff King @ 2016-09-29 18:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kyle J. McKay, Git mailing list
In-Reply-To: <xmqqtwcyfvfz.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 10:49:04AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > This lets you stick a header right before a commit, but
> > suppresses headers that are duplicates. This means you can
> > do something like:
> >
> > git log --graph --author-date-order --commit-header='== %as =='
> >
> > to get a marker in the graph whenever the day changes.
>
> That's interesting. So it is not really "commit" header, but a
> header for groups of commits. Credits for realizing the usefulness
> of such grouping may go to Kyle, but the implementation is also
> brilliant ;-).
Yeah, I really don't like the name "--commit-header" that much. I
initially thought to call it "--graph-header", but it is potentially
useful without a graph, too. Maybe "--group-header" or something.
I dunno. I'd leave that to somebody who actually wanted to polish the
patches up enough for submission. That might even be me someday, but not
today. :)
-Peff
^ permalink raw reply
* Re: [PATCH/RFC] git log --oneline alternative with dates, times and initials
From: Jeff King @ 2016-09-29 18:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kyle J. McKay, Git mailing list
In-Reply-To: <xmqqy42afvy1.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 10:38:14AM -0700, Junio C Hamano wrote:
> > I have no problem taking this in contrib or whatever, until a point when
> > Git is capable of doing the same thing itself. I just hoped to trick you
> > into working on Git. :)
>
> I thought we stopped adding random things to contrib/, though.
>
> Unlike the earlier days of Git, if a custom command that uses Git is
> very userful, it can live its own life and flourish within the much
> larger Git userbase we have these days.
I dunno. I said "contrib or whatever" to duck that question. :)
I do not have a strong opinion either way. In some ways this script is
similar to diff-highlight, which is in contrib. Perhaps that is only
because diff-highlight is grandfathered. But I also think it somewhat
makes sense, because in an ideal world diff-highlight gets thrown away
in favor of git's internal diff routines learning to do the same thing.
And in theory this script is in the same position.
But I also buy the argument that contrib/ is simply a hassle. This
script can live in its own repository somewhere, and handle
announcements and patches on the list. For that matter, so could
diff-highlight, and I don't mind ripping it out of contrib if that's the
consensus. My only real objection is that doing so is more work than
leaving it as-is, and I'm lazy.
-Peff
^ permalink raw reply
* Re: [PATCH 2/4] t13xx: do not assume system config is empty
From: Jeff King @ 2016-09-29 18:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, torvalds
In-Reply-To: <xmqqintefuau.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 11:13:45AM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I think anytime you would use GIT_CONFIG_NOSYSTEM over --local, it is an
> > indication that the test is trying to check how multiple sources
> > interact. And the right thing to do for them is to set GIT_ETC_GITCONFIG
> > to some known quantity. We just couldn't do that before, so we skipped
> > it. IOW, something like the patch below (on top of yours).
>
> OK, that way we can make sure that "multiple sources" operations do
> look at the system-wide stuff.
Exactly.
> > Note that the
> > commands that are doing a "--get" and not a "--list" don't actually seem
> > to need either (because they are getting the values out of the local
> > file anyway), so we could drop the setting of GIT_ETC_GITCONFIG from
> > them entirely.
>
> "either" meaning "we do not need to add --local and we do not need
> GIT_CONFIG_NOSYSTEM"?
Yes. I didn't test it with your core.abbrev patch 4/4, but I _didn't_
have to touch their expected output after pointing them at a non-empty
etc-gitconfig file in the trash directory. Which implies to me they
don't care either way (which makes sense; they are asking for a specific
key which is supposed to be found in one of the other files).
-Peff
^ permalink raw reply
* Re: [PATCH v8 00/11] Git filter protocol
From: Torsten Bögershausen @ 2016-09-29 18:18 UTC (permalink / raw)
To: Lars Schneider, Junio C Hamano
Cc: git, Jeff King, Stefan Beller, Jakub Narębski,
Martin-Louis Bright, ramsay
In-Reply-To: <1A8A9127-4DF9-44AD-9497-F8A630AB1193@gmail.com>
On 29/09/16 19:57, Lars Schneider wrote:
>> On 29 Sep 2016, at 18:57, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Torsten Bögershausen <tboegi@web.de> writes:
>>
>>>> 1) Git exits
>>>> 2) The filter process receives EOF and prints "STOP" to the log
>>>> 3) t0021 checks the content of the log
>>>>
>>>> Sometimes 3 happened before 2 which makes the test fail.
>>>> (Example: https://travis-ci.org/git/git/jobs/162660563 )
>>>>
>>>> I added a this to wait until the filter process terminates:
>>>>
>>>> +wait_for_filter_termination () {
>>>> + while ps | grep -v grep | grep -F "/t0021/rot13-filter.pl" >/dev/null 2>&1
>>>> + do
>>>> + echo "Waiting for /t0021/rot13-filter.pl to finish..."
>>>> + sleep 1
>>>> + done
>>>> +}
>>>>
>>>> Does this look OK to you?
>>> Do we need the ps at all ?
>>> How about this:
>>>
>>> +wait_for_filter_termination () {
>>> + while ! grep "STOP" LOGFILENAME >/dev/null
>>> + do
>>> + echo "Waiting for /t0021/rot13-filter.pl to finish..."
>>> + sleep 1
>>> + done
>>> +}
>> Running "ps" and grepping for a command is not suitable for script
>> to reliably tell things, so it is out of question. Compared to
>> that, your version looks slightly better, but what if the machinery
>> that being tested, i.e. the part that drives the filter process, is
>> buggy or becomes buggy and causes the filter process that writes
>> "STOP" to die before it actually writes that string?
>>
>> I have a feeling that the machinery being tested needs to be fixed
>> so that the sequence is always be:
>>
>> 0) Git spawns the filter process, as it needs some contents to
>> be filtered.
>>
>> 1) Git did everything it needed to do and decides that is time
>> to go.
>>
>> 2) Filter process receives EOF and prints "STOP" to the log.
>>
>> 3) Git waits until the filter process finishes.
>>
>> 4) t0021, after Git finishes, checks the log.
>>
>> Repeated sleep combined with grep is probably just sweeping the real
>> problem under the rug. Do we have enough information to do the
>> above?
>>
>> An inspiration may be in the way we centrally clean all tempfiles
>> and lockfiles before exiting. We have a central registry of these
>> files that need cleaning up and have a single atexit(3) handler to
>> clean them up. Perhaps we need a registry that filter processes
>> spawned by the mechanism Lars introduces in this series, and have an
>> atexit(3) handler that closes the pipe to them (which signals the
>> filters that it is time for them to go) and wait(2) on them, or
>> something? I do not think we want any kill(2) to be involved in
>> this clean-up procedure, but I do think we should wait(2) on what we
>> spawn, as long as these processes are meant to be shut down when the
>> main process of Git exits (this is different from things like
>> credential-cache daemon where they are expected to persist and meant
>> to serve multiple Git processes).
> We discussed that issue in v4 and v6:
> http://public-inbox.org/git/20160803225313.pk3tfe5ovz4y3i7l@sigill.intra.peff.net/
> http://public-inbox.org/git/xmqqbn0a3wy3.fsf@gitster.mtv.corp.google.com/
>
> My impression was that you don't want Git to wait for the filter process.
> If Git waits for the filter process - how long should Git wait?
>
> Thanks,
> Lars
Hm,
I would agree that Git should not wait for the filter.
But does the test suite need to wait for the filter ?
May be, in this case we test the filter and Git, which is good.
Adding a 1 second delay, if, and only if, there is a racy condition,
is not that bad (or do we have better ways to check for a process to
be terminated ?)
^ permalink raw reply
* Re: [PATCH 2/4] t13xx: do not assume system config is empty
From: Junio C Hamano @ 2016-09-29 18:13 UTC (permalink / raw)
To: Jeff King; +Cc: git, torvalds
In-Reply-To: <20160929090108.hf2jzfcvbcsfaxw7@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think anytime you would use GIT_CONFIG_NOSYSTEM over --local, it is an
> indication that the test is trying to check how multiple sources
> interact. And the right thing to do for them is to set GIT_ETC_GITCONFIG
> to some known quantity. We just couldn't do that before, so we skipped
> it. IOW, something like the patch below (on top of yours).
OK, that way we can make sure that "multiple sources" operations do
look at the system-wide stuff.
> Note that the
> commands that are doing a "--get" and not a "--list" don't actually seem
> to need either (because they are getting the values out of the local
> file anyway), so we could drop the setting of GIT_ETC_GITCONFIG from
> them entirely.
"either" meaning "we do not need to add --local and we do not need
GIT_CONFIG_NOSYSTEM"?
^ permalink raw reply
* Re: Two bugs in --pretty with %C(auto)
From: René Scharfe @ 2016-09-29 18:13 UTC (permalink / raw)
To: Anatoly Borodin, Duy Nguyen; +Cc: git, Junio C Hamano
In-Reply-To: <db20ae0c-9c33-1e65-b201-1b6a9ed11340@web.de>
Am 17.09.2016 um 20:25 schrieb René Scharfe:
> diff --git a/pretty.c b/pretty.c
> index 9788bd8..493edb0 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1072,6 +1072,8 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
> case 'C':
> if (starts_with(placeholder + 1, "(auto)")) {
> c->auto_color = want_color(c->pretty_ctx->color);
> + if (c->auto_color)
> + strbuf_addstr(sb, GIT_COLOR_RESET);
> return 7; /* consumed 7 bytes, "C(auto)" */
> } else {
> int ret = parse_color(sb, placeholder, c);
We could optimize this a bit (see below). I can't think of a downside;
someone adding a prefix would be responsible for adding a reset as well
if needed, right?
-- >8 --
Subject: [PATCH] pretty: avoid adding reset for %C(auto) if output is empty
We emit an escape sequence for resetting color and attribute for
%C(auto) to make sure automatic coloring is displayed as intended.
Stop doing that if the output strbuf is empty, i.e. when %C(auto)
appears at the start of the format string, because then there is no
need for a reset and we save a few bytes in the output.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
Reverts the change to t6006, so we'd need another test for this.
Anatoly? :)
pretty.c | 2 +-
t/t6006-rev-list-format.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/pretty.c b/pretty.c
index 493edb0..25efbca 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1072,7 +1072,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
case 'C':
if (starts_with(placeholder + 1, "(auto)")) {
c->auto_color = want_color(c->pretty_ctx->color);
- if (c->auto_color)
+ if (c->auto_color && sb->len)
strbuf_addstr(sb, GIT_COLOR_RESET);
return 7; /* consumed 7 bytes, "C(auto)" */
} else {
diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index f6020cd..a1dcdb8 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -225,7 +225,7 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' '
test_expect_success '%C(auto) respects --color' '
git log --color --format="%C(auto)%H" -1 >actual &&
- printf "\\033[m\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
+ printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect &&
test_cmp expect actual
'
--
2.10.0
^ permalink raw reply related
* Re: [PATCH 4/4] core.abbrev: raise the default abbreviation to 12 hexdigits
From: Junio C Hamano @ 2016-09-29 18:05 UTC (permalink / raw)
To: Johannes Sixt; +Cc: git, peff, torvalds
In-Reply-To: <ae9dbf3b-4190-8145-a59f-0d578067032a@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 29.09.2016 um 01:30 schrieb Junio C Hamano:
>> As Peff said, responding in a thread started by Linus's suggestion
>> to raise the default abbreviation to 12 hexdigits:
>
> This is waayy too large for a new default. The vast majority of
> repositories is smallish. For those, the long sequences of hex digits
> are an uglification that is almost unbearable.
>
> I know that kernel developers are important, but their importance has
> long been outnumbered by the anonymous and silent masses of users.
>
> Personally, I use 8 digits just because it is a "rounder" number than
> 7, but in all of my repositories 7 would still work just as well.
Yes, "git log --oneline" looks somewhat different and strange for
me, too ;-)
I am sure I'll get used to it if I keep using it, but I suspect that
I'd be irritated as I find myself typing 'q' more and more often to
"less -S" that is automatically invoked when I do "git log --oneline
master.." to see what commits are on my current topic branch.
^ permalink raw reply
* Re: [PATCH v8 00/11] Git filter protocol
From: Jeff King @ 2016-09-29 18:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: Torsten Bögershausen, Lars Schneider, git, Stefan Beller,
Jakub Narębski, Martin-Louis Bright, ramsay
In-Reply-To: <xmqqk2duhcdm.fsf@gitster.mtv.corp.google.com>
On Thu, Sep 29, 2016 at 09:57:57AM -0700, Junio C Hamano wrote:
> > +wait_for_filter_termination () {
> > + while ! grep "STOP" LOGFILENAME >/dev/null
> > + do
> > + echo "Waiting for /t0021/rot13-filter.pl to finish..."
> > + sleep 1
> > + done
> > +}
>
> Running "ps" and grepping for a command is not suitable for script
> to reliably tell things, so it is out of question. Compared to
> that, your version looks slightly better, but what if the machinery
> that being tested, i.e. the part that drives the filter process, is
> buggy or becomes buggy and causes the filter process that writes
> "STOP" to die before it actually writes that string?
I'm of the opinion that any busy-waiting is a good sign that something
is suboptimal. The right solution here seems like it should be signaling
the test script via a descriptor.
I don't necessarily agree, though, that the timing of filter-process
cleanup needs to be part of the public interface. So in your list:
> 3) Git waits until the filter process finishes.
That seems simple and elegant, but I can think of reasons we might not
want to wait (e.g., if the filter has to do some maintenance task and
does not the user to have to wait).
OTOH, we already face this in git, and we solve it by explicitly
backgrounding the maintenance task (i.e., auto-gc). So one could argue
that it is the responsibility of the filter process to manage its own
processes. It certainly makes the interaction with git simpler.
-Peff
^ permalink raw reply
* Re: [PATCH v8 00/11] Git filter protocol
From: Lars Schneider @ 2016-09-29 17:57 UTC (permalink / raw)
To: Junio C Hamano
Cc: Torsten Bögershausen, git, Jeff King, Stefan Beller,
Jakub Narębski, Martin-Louis Bright, ramsay
In-Reply-To: <xmqqk2duhcdm.fsf@gitster.mtv.corp.google.com>
> On 29 Sep 2016, at 18:57, Junio C Hamano <gitster@pobox.com> wrote:
>
> Torsten Bögershausen <tboegi@web.de> writes:
>
>>> 1) Git exits
>>> 2) The filter process receives EOF and prints "STOP" to the log
>>> 3) t0021 checks the content of the log
>>>
>>> Sometimes 3 happened before 2 which makes the test fail.
>>> (Example: https://travis-ci.org/git/git/jobs/162660563 )
>>>
>>> I added a this to wait until the filter process terminates:
>>>
>>> +wait_for_filter_termination () {
>>> + while ps | grep -v grep | grep -F "/t0021/rot13-filter.pl" >/dev/null 2>&1
>>> + do
>>> + echo "Waiting for /t0021/rot13-filter.pl to finish..."
>>> + sleep 1
>>> + done
>>> +}
>>>
>>> Does this look OK to you?
>> Do we need the ps at all ?
>> How about this:
>>
>> +wait_for_filter_termination () {
>> + while ! grep "STOP" LOGFILENAME >/dev/null
>> + do
>> + echo "Waiting for /t0021/rot13-filter.pl to finish..."
>> + sleep 1
>> + done
>> +}
>
> Running "ps" and grepping for a command is not suitable for script
> to reliably tell things, so it is out of question. Compared to
> that, your version looks slightly better, but what if the machinery
> that being tested, i.e. the part that drives the filter process, is
> buggy or becomes buggy and causes the filter process that writes
> "STOP" to die before it actually writes that string?
>
> I have a feeling that the machinery being tested needs to be fixed
> so that the sequence is always be:
>
> 0) Git spawns the filter process, as it needs some contents to
> be filtered.
>
> 1) Git did everything it needed to do and decides that is time
> to go.
>
> 2) Filter process receives EOF and prints "STOP" to the log.
>
> 3) Git waits until the filter process finishes.
>
> 4) t0021, after Git finishes, checks the log.
>
> Repeated sleep combined with grep is probably just sweeping the real
> problem under the rug. Do we have enough information to do the
> above?
>
> An inspiration may be in the way we centrally clean all tempfiles
> and lockfiles before exiting. We have a central registry of these
> files that need cleaning up and have a single atexit(3) handler to
> clean them up. Perhaps we need a registry that filter processes
> spawned by the mechanism Lars introduces in this series, and have an
> atexit(3) handler that closes the pipe to them (which signals the
> filters that it is time for them to go) and wait(2) on them, or
> something? I do not think we want any kill(2) to be involved in
> this clean-up procedure, but I do think we should wait(2) on what we
> spawn, as long as these processes are meant to be shut down when the
> main process of Git exits (this is different from things like
> credential-cache daemon where they are expected to persist and meant
> to serve multiple Git processes).
We discussed that issue in v4 and v6:
http://public-inbox.org/git/20160803225313.pk3tfe5ovz4y3i7l@sigill.intra.peff.net/
http://public-inbox.org/git/xmqqbn0a3wy3.fsf@gitster.mtv.corp.google.com/
My impression was that you don't want Git to wait for the filter process.
If Git waits for the filter process - how long should Git wait?
Thanks,
Lars
^ permalink raw reply
* Re: [PATCH 5/5] log: add --commit-header option
From: Junio C Hamano @ 2016-09-29 17:49 UTC (permalink / raw)
To: Jeff King; +Cc: Kyle J. McKay, Git mailing list
In-Reply-To: <20160929083851.kx6itvrh4n2rttrx@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This lets you stick a header right before a commit, but
> suppresses headers that are duplicates. This means you can
> do something like:
>
> git log --graph --author-date-order --commit-header='== %as =='
>
> to get a marker in the graph whenever the day changes.
That's interesting. So it is not really "commit" header, but a
header for groups of commits. Credits for realizing the usefulness
of such grouping may go to Kyle, but the implementation is also
brilliant ;-).
> This probably needs some refactoring around the setup of the
> pretty-print context.
^ 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