* Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR
From: Junio C Hamano @ 2011-10-19 6:19 UTC (permalink / raw)
To: Michael Haggerty
Cc: Jeff King, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <7v39epft32.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I was trying to summarize this topic for Release Notes.
>
> Possibly incompatible changes
> -----------------------------
>
> * Special refs such as "HEAD", "MERGE_HEAD", and "FETCH_HEAD" are now
> restricted to names with only uppercase letters and underscore. All
> other refs must live under refs/ namespace. Earlier, you could
> confuse git by storing an object name in $GIT_DIR/tmp/junk and say
> "git show tmp/junk", but this will no longer work.
>
> But noticed that "git update-ref tmp/junk HEAD" does create such a ref
> that won't be recognized, and "git check-ref-format tmp/junk" is happy.
>
> I think we would need to restrict check_ref_format() so that these
> commands (and possibly others, but I think that single function will cover
> pretty much everything) also reject "tmp/junk" immediately below $GIT_DIR
> as a bad string. Otherwise we cannot merge these fixups, which would mean
> we would have to revert the "Clean up refname checks and normalization"
> series, at least the part that started emitting the "warning", which is a
> mess I would rather want to avoid.
>
> Opinions on how to get us out of this mess?
Addendum.
I was digging this further and see fairly large conflicts between the bulk
of "clean up refname checks and normalization" topic and the logic to
avoid the additional warning by tightening the dwimmery.
check_refname_format() has always assumed that it is OK to be called at
any level of substring, and there are many code like this one (example is
from remote.c::get_fetch_map()):
for (rmp = &ref_map; *rmp; ) {
if ((*rmp)->peer_ref) {
if (check_refname_format((*rmp)->peer_ref->name + 5,
REFNAME_ALLOW_ONELEVEL)) {
struct ref *ignore = *rmp;
error("* Ignoring funny ref '%s' locally",
(*rmp)->peer_ref->name);
This code somehow _knows_ that peer_ref->name begins with "refs/" and that
is the reason it adds 5 to skip that known part. In this particular case,
I think we can simply drop the +5 and allow-onelevel, but there are other
instances of the calls to the function that feeds the rest of the refname
string, skipping leading substring (not necessarily "refs/"), assuming
that any component string is either valid or invalid no matter where it
appears in the full refname. I wouldn't be surprised if some callers do
not even have enough information to tell what the leading substring would
be in the full refname context (e.g. parsing of "master:master" refspec,
relying on the later dwimmery to add refs/heads/ in front, could just
verify that "master" is in good format with allow-onelevel).
The new restriction bolted on to that logic in jc/check-ref-format-fixup
series to work around the new warning in 629cd3a (resolve_ref(): emit
warnings for improperly-formatted references, 2011-09-15) is incompatible
with the assumption, as we would need to check full refname, and treat the
first refname component very differently from other components. It has to
be either "refs" in multi-component refname, or all caps in a one-level
one, but the callers of check_refname_format() are not designed to feed
the full refname to begin with.
I am tempted to revert 629cd3a (resolve_ref(): emit warnings for
improperly-formatted references, 2011-09-15) that started giving the
warnings, and drop the jc/check-ref-format-fixup topic [*1*] altogether,
but that is a short-sighted workaround I would rather want to avoid. It
essentially declares that the "Clean up refname checks" topic was a
failure and did not manage to really clean things up.
A possible alternative might be to leave check_refname_format() and its
callers as they are, introduce check_full_refname() function that knows
the new restriction on top of check-ref-format-fixup, and use that in
lock_ref_sha1(), lock_any_ref_for_update() and is_refname_available()
[*2*]. That way, we can keep the potentially useful "ill-formed contents
in the ref" warning and avoid possible confusion caused by random files
that are directly under $GIT_DIR, which would be far more preferable in
the longer term.
Anybody wants to give it a try?
[Footnote]
*1* The topic is misnamed; it is about fixing dwimmery, not checking.
*2* This currently does not seem to check the well-formedness of the ref
it is creating at all; it should check the "ref", but not "oldref" to
allow renaming a malformed ref to correct earlier mistakes.
^ permalink raw reply
* Re: [PATCH 2/2] Restrict ref-like names immediately below $GIT_DIR
From: Junio C Hamano @ 2011-10-19 5:28 UTC (permalink / raw)
To: Michael Haggerty
Cc: Jeff King, git, cmn, A Large Angry SCM, Daniel Barkalow,
Sverre Rabbelier
In-Reply-To: <4E9609E3.1000300@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
>>> Nit: the seen_non_root_char variable can be replaced by an early "return
>>> 0" from the loop and "return 1" if the loop falls through.
>>
>> Hmm, I thought that would fail when you feed "refs/heads/master" to the
>> function.
>
> You're right. My brain must be scrambled from all of the rebasing that
> I have been doing ;-)
>
> How about adding
>
> /*
> * Accept strings that are either ALL_CAPS or include a '/'.
> */
>
> (I think the underscore is implied by the example, but the comment could
> be expanded if necessary to be explicit.)
I was trying to summarize this topic for Release Notes.
Possibly incompatible changes
-----------------------------
* Special refs such as "HEAD", "MERGE_HEAD", and "FETCH_HEAD" are now
restricted to names with only uppercase letters and underscore. All
other refs must live under refs/ namespace. Earlier, you could
confuse git by storing an object name in $GIT_DIR/tmp/junk and say
"git show tmp/junk", but this will no longer work.
But noticed that "git update-ref tmp/junk HEAD" does create such a ref
that won't be recognized, and "git check-ref-format tmp/junk" is happy.
I think we would need to restrict check_ref_format() so that these
commands (and possibly others, but I think that single function will cover
pretty much everything) also reject "tmp/junk" immediately below $GIT_DIR
as a bad string. Otherwise we cannot merge these fixups, which would mean
we would have to revert the "Clean up refname checks and normalization"
series, at least the part that started emitting the "warning", which is a
mess I would rather want to avoid.
Opinions on how to get us out of this mess?
^ permalink raw reply
* Re: Git --reference bug(?)
From: Michael Haggerty @ 2011-10-19 5:01 UTC (permalink / raw)
To: Andrea Gelmini; +Cc: gitster, git
In-Reply-To: <CAK-xaQaUxJ5c_kN48g7-J9fosDv6awbAFQSFLpF2fA+hc-i-MA@mail.gmail.com>
On 10/19/2011 12:04 AM, Andrea Gelmini wrote:
> git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> /tmp/3.1 --reference /home/gelma/dev/kernel/linus/
> Cloning into /tmp/3.1...
> fatal: Reference has invalid format: 'refs/tags/3.1.1.1^{}'
> fatal: The remote end hung up unexpectedly
The upstream repo reports what look like non-reference references:
$ git ls-remote --tags
git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git | head
5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c refs/tags/v2.6.11
c39ae07f393806ccf406ef966e9a15afc43cc36a refs/tags/v2.6.11^{}
5dc01c595e6c6ec9ccda4f6f69c131c0dd945f8c refs/tags/v2.6.11-tree
c39ae07f393806ccf406ef966e9a15afc43cc36a refs/tags/v2.6.11-tree^{}
26791a8bcf0e6d33f43aef7682bdb555236d56de refs/tags/v2.6.12
9ee1c939d1cb936b1f98e8d81aeffab57bae46ab refs/tags/v2.6.12^{}
9e734775f7c22d2f89943ad6c745571f1930105f refs/tags/v2.6.12-rc2
1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 refs/tags/v2.6.12-rc2^{}
0397236d43e48e821cce5bbe6a80a1a56bb7cc3a refs/tags/v2.6.12-rc3
a2755a80f40e5794ddc20e00f781af9d6320fafb refs/tags/v2.6.12-rc3^{}
[...]
I've never seen this format before; is this the remote protocol for
peeled refs or maybe the behavior of an old version of git?
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: What should "git fetch origin +next" should do?
From: Junio C Hamano @ 2011-10-19 4:31 UTC (permalink / raw)
To: Marc Branchaud; +Cc: Jeff King, git
In-Reply-To: <4E9CA5E2.2020701@xiplink.com>
Marc Branchaud <marcnarc@xiplink.com> writes:
> In all cases if the command-line refspec has no RHS then git should try to
> figure out which local ref to update from the config, and it should die if it
> can't figure out a local ref to create or update. (As I said above, maybe
> allow "git fetch origin foo:" to let the user put the tip of origin's foo ref
> into FETCH_HEAD.)
I'd agree with everything you said in your message, except for the above
"it should die" part.
You are assuming that the user knows what his configured refspecs would
normally do and that is the whole reason why "git fetch origin next" that
would update the remote tracking branch specified for the upstream's next
might feel more natural than the current behaviour. I too think that is a
reasonable assumption.
With the same assumption, if you said "git fetch origin frotz" when you
know you are not usually tracking the remote 'frotz' branch, the command
just should store what is fetched in FETCH_HEAD (and nowhere else) without
dying. I do not think how it helps the user to die in that situation.
> All this gets a bit more complicated if the user has currently checked out
> the a ref that should be updated (regardless of the presence of a LHS +).
That "do not update the currently checked out branch" already exists and
is correctly handled by "git fetch", I think.
^ permalink raw reply
* Re: [PATCHv6 0/5] Moving gitweb documentation to manpages
From: Junio C Hamano @ 2011-10-19 4:29 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git, Drew Northup, Jonathan Nieder
In-Reply-To: <1318763255-23495-1-git-send-email-jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> The original patch adding manpage for /etc/gitweb.conf was sent by
> Drew Northup (as can be seen from shortlog); I have added manpage for
> gitweb itself, inspired by his patch. Unfortunately Drew doesn't
> currently have time to work on this patch (patch series), so that is
> why it is me resending this series (yet again).
>
> The difference with previous version
>
> [PATCHv5/RFC 0/6] Moving gitweb documentation to manpages
> http://thread.gmane.org/gmane.comp.version-control.git/183166
> http://marc.info/?l=git&m=131809875619259&w=2
>
> is that "Documentation: Preparation for gitweb manpages" (originally a
> first patch) got removed, and two main patches got improved. Other
> patches are unchanged from previous version.
>
>
> Pull request:
> ~~~~~~~~~~~~~
> The following changes since commit 288396994f077eec7e7db0017838a5afbfbf81e3:
>
> Sync with maint (2011-10-15 20:59:50 -0700)
>
> are available in the git repository at:
>
> git://repo.or.cz/git/jnareb-git.git gitweb/doc
I fetched it and compared with the result of applying this series; it
appears that the version fetched from there is full of random whitespace
issues, e.g. ('$' added by "cat -e")
diff --git b/Documentation/gitweb.txt a/Documentation/gitweb.txt$
index 605a085..353e37f 100644$
--- b/Documentation/gitweb.txt$
+++ a/Documentation/gitweb.txt$
@@ -370,12 +370,12 @@ commitdiff::$
view shows information about commit in more detail, the 'commitdiff'$
action shows changeset for given commit.$
$
-patch::$
+patch:: $
Returns the commit in plain text mail format, suitable for applying with$
linkgit:git-am[1].$
The series seems to have been rebased on a more recent 'master' and does
not match the "since commit..." stated above (not a major offense; just
mentioning as I noticed it).
In any case, I've queued the one from the mailing list. Thanks.
^ permalink raw reply related
* [PATCH v3 3/3] log: --show-signature
From: Junio C Hamano @ 2011-10-19 0:20 UTC (permalink / raw)
To: git
In-Reply-To: <1318983645-18897-1-git-send-email-gitster@pobox.com>
This teaches the "log" family of commands to pass the GPG signature in the
commit objects to "gpg --verify" via the verify_signed_buffer() interface
used to verify signed tag objects. E.g.
$ git show --show-signature -s HEAD
would show GPG output in the header part of the output.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* We may want to capture good/bad/untrusted signature information from
GPG and teach userformat to show that, but this code is not there yet.
commit.c | 34 ++++++++++++++++++++++++++++++++++
commit.h | 3 +++
log-tree.c | 17 +++++++++++++++++
revision.c | 2 ++
revision.h | 1 +
5 files changed, 57 insertions(+), 0 deletions(-)
diff --git a/commit.c b/commit.c
index 4bff3cd..93045a2 100644
--- a/commit.c
+++ b/commit.c
@@ -848,6 +848,40 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)
return 0;
}
+int parse_signed_commit(const unsigned char *sha1,
+ struct strbuf *payload, struct strbuf *signature)
+{
+ unsigned long size;
+ enum object_type type;
+ char *buffer = read_sha1_file(sha1, &type, &size);
+ int in_header, saw_signature = -1;
+ char *line;
+
+ if (!buffer || type != OBJ_COMMIT)
+ goto cleanup;
+
+ line = buffer;
+ in_header = 1;
+ saw_signature = 0;
+ while (*line) {
+ char *next = strchrnul(line, '\n');
+ if (*next)
+ next++;
+ if (in_header && !prefixcmp(line, gpg_sig_header)) {
+ const char *sig = line + gpg_sig_header_len;
+ strbuf_add(signature, sig, next - sig);
+ saw_signature = 1;
+ } else {
+ strbuf_add(payload, line, next - line);
+ }
+ if (*line == '\n')
+ in_header = 0;
+ line = next;
+ }
+ cleanup:
+ free(buffer);
+ return saw_signature;
+}
static const char commit_utf8_warn[] =
"Warning: commit message does not conform to UTF-8.\n"
diff --git a/commit.h b/commit.h
index 8c2419b..1885471 100644
--- a/commit.h
+++ b/commit.h
@@ -177,4 +177,7 @@ extern int commit_tree(const char *msg, unsigned char *tree,
struct commit_list *parents, unsigned char *ret,
const char *author, const char *sign_commit);
+extern int parse_signed_commit(const unsigned char *sha1,
+ struct strbuf *message, struct strbuf *signature);
+
#endif /* COMMIT_H */
diff --git a/log-tree.c b/log-tree.c
index 24c295e..749bb65 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -8,6 +8,7 @@
#include "refs.h"
#include "string-list.h"
#include "color.h"
+#include "gpg-interface.h"
struct decoration name_decoration = { "object names" };
@@ -395,6 +396,19 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
*extra_headers_p = extra_headers;
}
+static void show_signature(struct rev_info *opt, struct commit *commit)
+{
+ struct strbuf payload = STRBUF_INIT;
+ struct strbuf signature = STRBUF_INIT;
+
+ if (parse_signed_commit(commit->object.sha1, &payload, &signature) >= 0) {
+ verify_signed_buffer(payload.buf, payload.len,
+ signature.buf, signature.len, 0);
+ }
+ strbuf_release(&payload);
+ strbuf_release(&signature);
+}
+
void show_log(struct rev_info *opt)
{
struct strbuf msgbuf = STRBUF_INIT;
@@ -502,6 +516,9 @@ void show_log(struct rev_info *opt)
}
}
+ if (opt->show_signature)
+ show_signature(opt, commit);
+
if (!commit->buffer)
return;
diff --git a/revision.c b/revision.c
index c46cfaa..860a312 100644
--- a/revision.c
+++ b/revision.c
@@ -1381,6 +1381,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->show_notes = 1;
revs->show_notes_given = 1;
revs->notes_opt.use_default_notes = 1;
+ } else if (!strcmp(arg, "--show-signature")) {
+ revs->show_signature = 1;
} else if (!prefixcmp(arg, "--show-notes=") ||
!prefixcmp(arg, "--notes=")) {
struct strbuf buf = STRBUF_INIT;
diff --git a/revision.h b/revision.h
index 3d64ada..198bb95 100644
--- a/revision.h
+++ b/revision.h
@@ -89,6 +89,7 @@ struct rev_info {
show_merge:1,
show_notes:1,
show_notes_given:1,
+ show_signature:1,
pretty_given:1,
abbrev_commit:1,
abbrev_commit_given:1,
--
1.7.7.388.g3a4b7
^ permalink raw reply related
* [PATCH v3 1/3] Split GPG interface into its own helper library
From: Junio C Hamano @ 2011-10-19 0:20 UTC (permalink / raw)
To: git
In-Reply-To: <1318983645-18897-1-git-send-email-gitster@pobox.com>
This mostly moves existing code from builtin/tag.c (for signing)
and builtin/verify-tag.c (for verifying) to a new gpg-interface.c
file to provide a more generic library interface.
- sign_buffer() takes a payload strbuf, a signature strbuf, and a signing
key, runs "gpg" to produce a detached signature for the payload, and
appends it to the signature strbuf. The contents of a signed tag that
concatenates the payload and the detached signature can be produced by
giving the same strbuf as payload and signature strbuf.
- verify_signed_buffer() takes a payload and a detached signature as
<ptr, len> pairs, and runs "gpg --verify" to see if the payload matches
the signature.
"verify-tag" (aka "tag -v") saved the whole tag contents as if it is a
detached signature, and fed gpg the payload part of the tag. It relied on
gpg to fail when the given tag is not signed but just is annotated. The
updated run_gpg_verify() function detects the lack of detached signature
in the input, and errors out without bothering "gpg".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Makefile | 2 +
builtin/tag.c | 76 ++-------------------------
builtin/verify-tag.c | 36 ++-----------
gpg-interface.c | 137 ++++++++++++++++++++++++++++++++++++++++++++++++++
gpg-interface.h | 10 ++++
tag.c | 5 ++
6 files changed, 166 insertions(+), 100 deletions(-)
create mode 100644 gpg-interface.c
create mode 100644 gpg-interface.h
diff --git a/Makefile b/Makefile
index 8d6d451..2183223 100644
--- a/Makefile
+++ b/Makefile
@@ -530,6 +530,7 @@ LIB_H += exec_cmd.h
LIB_H += fsck.h
LIB_H += gettext.h
LIB_H += git-compat-util.h
+LIB_H += gpg-interface.h
LIB_H += graph.h
LIB_H += grep.h
LIB_H += hash.h
@@ -620,6 +621,7 @@ LIB_OBJS += entry.o
LIB_OBJS += environment.o
LIB_OBJS += exec_cmd.o
LIB_OBJS += fsck.o
+LIB_OBJS += gpg-interface.o
LIB_OBJS += graph.o
LIB_OBJS += grep.o
LIB_OBJS += hash.o
diff --git a/builtin/tag.c b/builtin/tag.c
index 667515e..fb0d4a1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -14,6 +14,7 @@
#include "parse-options.h"
#include "diff.h"
#include "revision.h"
+#include "gpg-interface.h"
static const char * const git_tag_usage[] = {
"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
@@ -23,8 +24,6 @@ static const char * const git_tag_usage[] = {
NULL
};
-static char signingkey[1000];
-
struct tag_filter {
const char **patterns;
int lines;
@@ -208,60 +207,7 @@ static int verify_tag(const char *name, const char *ref,
static int do_sign(struct strbuf *buffer)
{
- struct child_process gpg;
- const char *args[4];
- char *bracket;
- int len;
- int i, j;
-
- if (!*signingkey) {
- if (strlcpy(signingkey, git_committer_info(IDENT_ERROR_ON_NO_NAME),
- sizeof(signingkey)) > sizeof(signingkey) - 1)
- return error(_("committer info too long."));
- bracket = strchr(signingkey, '>');
- if (bracket)
- bracket[1] = '\0';
- }
-
- /* When the username signingkey is bad, program could be terminated
- * because gpg exits without reading and then write gets SIGPIPE. */
- signal(SIGPIPE, SIG_IGN);
-
- memset(&gpg, 0, sizeof(gpg));
- gpg.argv = args;
- gpg.in = -1;
- gpg.out = -1;
- args[0] = "gpg";
- args[1] = "-bsau";
- args[2] = signingkey;
- args[3] = NULL;
-
- if (start_command(&gpg))
- return error(_("could not run gpg."));
-
- if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
- close(gpg.in);
- close(gpg.out);
- finish_command(&gpg);
- return error(_("gpg did not accept the tag data"));
- }
- close(gpg.in);
- len = strbuf_read(buffer, gpg.out, 1024);
- close(gpg.out);
-
- if (finish_command(&gpg) || !len || len < 0)
- return error(_("gpg failed to sign the tag"));
-
- /* Strip CR from the line endings, in case we are on Windows. */
- for (i = j = 0; i < buffer->len; i++)
- if (buffer->buf[i] != '\r') {
- if (i != j)
- buffer->buf[j] = buffer->buf[i];
- j++;
- }
- strbuf_setlen(buffer, j);
-
- return 0;
+ return sign_buffer(buffer, buffer, get_signing_key());
}
static const char tag_template[] =
@@ -270,21 +216,11 @@ static const char tag_template[] =
"# Write a tag message\n"
"#\n");
-static void set_signingkey(const char *value)
-{
- if (strlcpy(signingkey, value, sizeof(signingkey)) >= sizeof(signingkey))
- die(_("signing key value too long (%.10s...)"), value);
-}
-
static int git_tag_config(const char *var, const char *value, void *cb)
{
- if (!strcmp(var, "user.signingkey")) {
- if (!value)
- return config_error_nonbool(var);
- set_signingkey(value);
- return 0;
- }
-
+ int status = git_gpg_config(var, value, cb);
+ if (status)
+ return status;
return git_default_config(var, value, cb);
}
@@ -463,7 +399,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (keyid) {
sign = 1;
- set_signingkey(keyid);
+ set_signing_key(keyid);
}
if (sign)
annotate = 1;
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 3134766..4bbcf77 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -11,6 +11,7 @@
#include "run-command.h"
#include <signal.h>
#include "parse-options.h"
+#include "gpg-interface.h"
static const char * const verify_tag_usage[] = {
"git verify-tag [-v|--verbose] <tag>...",
@@ -19,42 +20,17 @@ static const char * const verify_tag_usage[] = {
static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
{
- struct child_process gpg;
- const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
- char path[PATH_MAX];
- size_t len;
- int fd, ret;
+ int len;
- fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
- if (fd < 0)
- return error("could not create temporary file '%s': %s",
- path, strerror(errno));
- if (write_in_full(fd, buf, size) < 0)
- return error("failed writing temporary file '%s': %s",
- path, strerror(errno));
- close(fd);
-
- /* find the length without signature */
len = parse_signature(buf, size);
if (verbose)
write_in_full(1, buf, len);
- memset(&gpg, 0, sizeof(gpg));
- gpg.argv = args_gpg;
- gpg.in = -1;
- args_gpg[2] = path;
- if (start_command(&gpg)) {
- unlink(path);
- return error("could not run gpg.");
- }
-
- write_in_full(gpg.in, buf, len);
- close(gpg.in);
- ret = finish_command(&gpg);
+ if (size == len)
+ return error("no signature found");
- unlink_or_warn(path);
-
- return ret;
+ return verify_signed_buffer(buf, len,
+ buf + len, size - len, 0);
}
static int verify_tag(const char *name, int verbose)
diff --git a/gpg-interface.c b/gpg-interface.c
new file mode 100644
index 0000000..62f3806
--- /dev/null
+++ b/gpg-interface.c
@@ -0,0 +1,137 @@
+#include "cache.h"
+#include "run-command.h"
+#include "strbuf.h"
+#include "gpg-interface.h"
+#include "sigchain.h"
+
+static char *configured_signing_key;
+
+void set_signing_key(const char *key)
+{
+ free(configured_signing_key);
+ configured_signing_key = xstrdup(key);
+}
+
+int git_gpg_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, "user.signingkey")) {
+ if (!value)
+ return config_error_nonbool(var);
+ set_signing_key(value);
+ }
+ return 0;
+}
+
+const char *get_signing_key(void)
+{
+ if (configured_signing_key)
+ return configured_signing_key;
+ return git_committer_info(IDENT_ERROR_ON_NO_NAME|IDENT_NO_DATE);
+}
+
+/*
+ * Create a detached signature for the contents of "buffer" and append
+ * it after "signature"; "buffer" and "signature" can be the same
+ * strbuf instance, which would cause the detached signature appended
+ * at the end.
+ */
+int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key)
+{
+ struct child_process gpg;
+ const char *args[4];
+ ssize_t len;
+ size_t i, j, bottom;
+
+ memset(&gpg, 0, sizeof(gpg));
+ gpg.argv = args;
+ gpg.in = -1;
+ gpg.out = -1;
+ args[0] = "gpg";
+ args[1] = "-bsau";
+ args[2] = signing_key;
+ args[3] = NULL;
+
+ if (start_command(&gpg))
+ return error(_("could not run gpg."));
+
+ /*
+ * When the username signingkey is bad, program could be terminated
+ * because gpg exits without reading and then write gets SIGPIPE.
+ */
+ sigchain_push(SIGPIPE, SIG_IGN);
+
+ if (write_in_full(gpg.in, buffer->buf, buffer->len) != buffer->len) {
+ close(gpg.in);
+ close(gpg.out);
+ finish_command(&gpg);
+ return error(_("gpg did not accept the data"));
+ }
+ close(gpg.in);
+
+ bottom = signature->len;
+ len = strbuf_read(signature, gpg.out, 1024);
+ close(gpg.out);
+
+ sigchain_pop(SIGPIPE);
+
+ if (finish_command(&gpg) || !len || len < 0)
+ return error(_("gpg failed to sign the data"));
+
+ /* Strip CR from the line endings, in case we are on Windows. */
+ for (i = j = bottom; i < signature->len; i++)
+ if (signature->buf[i] != '\r') {
+ if (i != j)
+ signature->buf[j] = signature->buf[i];
+ j++;
+ }
+ strbuf_setlen(signature, j);
+
+ return 0;
+}
+
+/*
+ * Run "gpg" to see if the payload matches the detached signature.
+ * gpg_output_to tells where the output from "gpg" should go:
+ * < 0: /dev/null
+ * = 0: standard error of the calling process
+ * > 0: the specified file descriptor
+ */
+int verify_signed_buffer(const char *payload, size_t payload_size,
+ const char *signature, size_t signature_size,
+ int gpg_output_to)
+{
+ struct child_process gpg;
+ const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
+ char path[PATH_MAX];
+ int fd, ret;
+
+ fd = git_mkstemp(path, PATH_MAX, ".git_vtag_tmpXXXXXX");
+ if (fd < 0)
+ return error("could not create temporary file '%s': %s",
+ path, strerror(errno));
+ if (write_in_full(fd, signature, signature_size) < 0)
+ return error("failed writing detached signature to '%s': %s",
+ path, strerror(errno));
+ close(fd);
+
+ memset(&gpg, 0, sizeof(gpg));
+ gpg.argv = args_gpg;
+ gpg.in = -1;
+ if (gpg_output_to < 0)
+ gpg.no_stderr = 1;
+ else
+ gpg.err = gpg_output_to;
+ args_gpg[2] = path;
+ if (start_command(&gpg)) {
+ unlink(path);
+ return error("could not run gpg.");
+ }
+
+ write_in_full(gpg.in, payload, payload_size);
+ close(gpg.in);
+ ret = finish_command(&gpg);
+
+ unlink_or_warn(path);
+
+ return ret;
+}
diff --git a/gpg-interface.h b/gpg-interface.h
new file mode 100644
index 0000000..df58bb9
--- /dev/null
+++ b/gpg-interface.h
@@ -0,0 +1,10 @@
+#ifndef GPG_INTERFACE_H
+#define GPG_INTERFACE_H
+
+extern int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *signing_key);
+extern int verify_signed_buffer(const char *payload, size_t payload_size, const char *signature, size_t signature_size, int gpg_output_to);
+extern int git_gpg_config(const char *, const char *, void *);
+extern void set_signing_key(const char *);
+extern const char *get_signing_key(void);
+
+#endif
diff --git a/tag.c b/tag.c
index 7d38cc0..3aa186d 100644
--- a/tag.c
+++ b/tag.c
@@ -139,6 +139,11 @@ int parse_tag(struct tag *item)
return ret;
}
+/*
+ * Look at a signed tag object, and return the offset where
+ * the embedded detached signature begins, or the end of the
+ * data when there is no such signature.
+ */
size_t parse_signature(const char *buf, unsigned long size)
{
char *eol;
--
1.7.7.388.g3a4b7
^ permalink raw reply related
* [PATCH v3 2/3] commit: teach --gpg-sign option
From: Junio C Hamano @ 2011-10-19 0:20 UTC (permalink / raw)
To: git
In-Reply-To: <1318983645-18897-1-git-send-email-gitster@pobox.com>
And this uses the gpg-interface.[ch] to allow signing the commit, i.e.
$ git commit --gpg-sign -m foo
You need a passphrase to unlock the secret key for
user: "Junio C Hamano <gitster@pobox.com>"
4096-bit RSA key, ID 96AFE6CB, created 2011-10-03 (main key ID 713660A7)
[master 8457d13] foo
1 files changed, 1 insertions(+), 0 deletions(-)
The lines of GPG detached signature are placed in new header lines, after
the standard tree/parent/author/committer headers, instead of tucking the
signature block at the end of the commit log message text (similar to how
signed tag is done), for multiple reasons:
- The signature won't clutter output from "git log" and friends if it is
in the extra header. If we place it at the end of the log message, we
would need to teach "git log" and friends to strip the signature block
with an option.
- Teaching new versions of "git log" and "gitk" to optionally verify and
show signatures is cleaner if we structurally know where the signature
block is (instead of scanning in the commit log message).
- The signature needs to be stripped upon various commit rewriting
operations, e.g. rebase, filter-branch, etc. They all already ignore
unknown headers, but if we place signature in the log message, all of
these tools (and third-party tools) also need to learn how a signature
block would look like.
- When we added the optional encoding header, all the tools (both in tree
and third-party) that acts on the raw commit object should have been
fixed to ignore headers they do not understand, so it is not like that
new header would be more likely to break than extra text in the commit.
A commit made with the above sample sequence would look like this:
$ git cat-file commit HEAD
tree 3cd71d90e3db4136e5260ab54599791c4f883b9d
parent b87755351a47b09cb27d6913e6e0e17e6254a4d4
author Junio C Hamano <gitster@pobox.com> 1317862251 -0700
committer Junio C Hamano <gitster@pobox.com> 1317862251 -0700
gpgsig -----BEGIN PGP SIGNATURE-----
gpgsig Version: GnuPG v1.4.10 (GNU/Linux)
gpgsig
gpgsig iQIcBAABAgAGBQJOjPtrAAoJELC16IaWr+bL4TMP/RSe2Y/jYnCkds9unO5JEnfG
gpgsig ...
gpgsig =dt98
gpgsig -----END PGP SIGNATURE-----
foo
but "git log" (unless you ask for it with --pretty=raw) output is not
cluttered with the signature information.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* The header field was updated from "sig" to "gpgsig" so that later we
won't have to regret the choice when we introduce different kind of
signature mechanism.
builtin/commit-tree.c | 24 +++++++++++++++++++++---
builtin/commit.c | 12 ++++++++++--
builtin/merge.c | 16 ++++++++++++++--
commit.c | 40 +++++++++++++++++++++++++++++++++++++++-
commit.h | 2 +-
notes-cache.c | 2 +-
notes-merge.c | 2 +-
7 files changed, 87 insertions(+), 11 deletions(-)
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index d083795..a17811f 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -8,8 +8,9 @@
#include "tree.h"
#include "builtin.h"
#include "utf8.h"
+#include "gpg-interface.h"
-static const char commit_tree_usage[] = "git commit-tree <sha1> [(-p <sha1>)...] < changelog";
+static const char commit_tree_usage[] = "git commit-tree [-S<signer>] <sha1> [(-p <sha1>)...] < changelog";
static void new_parent(struct commit *parent, struct commit_list **parents_p)
{
@@ -25,6 +26,14 @@ static void new_parent(struct commit *parent, struct commit_list **parents_p)
commit_list_insert(parent, parents_p);
}
+static int commit_tree_config(const char *var, const char *value, void *cb)
+{
+ int status = git_gpg_config(var, value, NULL);
+ if (status)
+ return status;
+ return git_default_config(var, value, cb);
+}
+
int cmd_commit_tree(int argc, const char **argv, const char *prefix)
{
int i;
@@ -32,11 +41,19 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
unsigned char tree_sha1[20];
unsigned char commit_sha1[20];
struct strbuf buffer = STRBUF_INIT;
+ const char *sign_commit = NULL;
- git_config(git_default_config, NULL);
+ git_config(commit_tree_config, NULL);
if (argc < 2 || !strcmp(argv[1], "-h"))
usage(commit_tree_usage);
+
+ if (!memcmp(argv[1], "-S", 2)) {
+ sign_commit = argv[1] + 2;
+ argv++;
+ argc--;
+ }
+
if (get_sha1(argv[1], tree_sha1))
die("Not a valid object name %s", argv[1]);
@@ -56,7 +73,8 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix)
if (strbuf_read(&buffer, 0, 0) < 0)
die_errno("git commit-tree: failed to read");
- if (commit_tree(buffer.buf, tree_sha1, parents, commit_sha1, NULL)) {
+ if (commit_tree(buffer.buf, tree_sha1, parents, commit_sha1,
+ NULL, sign_commit)) {
strbuf_release(&buffer);
return 1;
}
diff --git a/builtin/commit.c b/builtin/commit.c
index cbc9613..90cf7e8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -26,6 +26,7 @@
#include "unpack-trees.h"
#include "quote.h"
#include "submodule.h"
+#include "gpg-interface.h"
static const char * const builtin_commit_usage[] = {
"git commit [options] [--] <filepattern>...",
@@ -85,6 +86,8 @@ static int all, edit_flag, also, interactive, patch_interactive, only, amend, si
static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship;
static int no_post_rewrite, allow_empty_message;
static char *untracked_files_arg, *force_date, *ignore_submodule_arg;
+static char *sign_commit;
+
/*
* The default commit message cleanup mode will remove the lines
* beginning with # (shell comments) and leading and trailing
@@ -144,6 +147,8 @@ static struct option builtin_commit_options[] = {
OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),
OPT_STRING(0, "cleanup", &cleanup_arg, "default", "how to strip spaces and #comments from message"),
OPT_BOOLEAN(0, "status", &include_status, "include status in commit message template"),
+ { OPTION_STRING, 'S', "gpg-sign", &sign_commit, "key id",
+ "GPG sign commit", PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
/* end commit message options */
OPT_GROUP("Commit contents options"),
@@ -1323,6 +1328,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
static int git_commit_config(const char *k, const char *v, void *cb)
{
struct wt_status *s = cb;
+ int status;
if (!strcmp(k, "commit.template"))
return git_config_pathname(&template_file, k, v);
@@ -1330,7 +1336,9 @@ static int git_commit_config(const char *k, const char *v, void *cb)
include_status = git_config_bool(k, v);
return 0;
}
-
+ status = git_gpg_config(k, v, NULL);
+ if (status)
+ return status;
return git_status_config(k, v, s);
}
@@ -1481,7 +1489,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
}
if (commit_tree(sb.buf, active_cache_tree->sha1, parents, sha1,
- author_ident.buf)) {
+ author_ident.buf, sign_commit)) {
rollback_index_files();
die(_("failed to write commit object"));
}
diff --git a/builtin/merge.c b/builtin/merge.c
index ab4077f..53cff02 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -26,6 +26,7 @@
#include "merge-recursive.h"
#include "resolve-undo.h"
#include "remote.h"
+#include "gpg-interface.h"
#define DEFAULT_TWOHEAD (1<<0)
#define DEFAULT_OCTOPUS (1<<1)
@@ -63,6 +64,7 @@ static int allow_rerere_auto;
static int abort_current_merge;
static int show_progress = -1;
static int default_to_upstream;
+static const char *sign_commit;
static struct strategy all_strategy[] = {
{ "recursive", DEFAULT_TWOHEAD | NO_TRIVIAL },
@@ -206,6 +208,8 @@ static struct option builtin_merge_options[] = {
OPT_BOOLEAN(0, "abort", &abort_current_merge,
"abort the current in-progress merge"),
OPT_SET_INT(0, "progress", &show_progress, "force progress reporting", 1),
+ { OPTION_STRING, 'S', "gpg-sign", &sign_commit, "key id",
+ "GPG sign commit", PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_END()
};
@@ -525,6 +529,8 @@ static void parse_branch_merge_options(char *bmo)
static int git_merge_config(const char *k, const char *v, void *cb)
{
+ int status;
+
if (branch && !prefixcmp(k, "branch.") &&
!prefixcmp(k + 7, branch) &&
!strcmp(k + 7 + strlen(branch), ".mergeoptions")) {
@@ -562,6 +568,10 @@ static int git_merge_config(const char *k, const char *v, void *cb)
default_to_upstream = git_config_bool(k, v);
return 0;
}
+
+ status = git_gpg_config(k, v, NULL);
+ if (status)
+ return status;
return git_diff_ui_config(k, v, cb);
}
@@ -870,7 +880,8 @@ static int merge_trivial(void)
parent->next->item = remoteheads->item;
parent->next->next = NULL;
run_prepare_commit_msg();
- commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL);
+ commit_tree(merge_msg.buf, result_tree, parent, result_commit, NULL,
+ sign_commit);
finish(result_commit, "In-index merge");
drop_save();
return 0;
@@ -900,7 +911,8 @@ static int finish_automerge(struct commit_list *common,
free_commit_list(remoteheads);
strbuf_addch(&merge_msg, '\n');
run_prepare_commit_msg();
- commit_tree(merge_msg.buf, result_tree, parents, result_commit, NULL);
+ commit_tree(merge_msg.buf, result_tree, parents, result_commit,
+ NULL, sign_commit);
strbuf_addf(&buf, "Merge made by the '%s' strategy.", wt_strategy);
finish(result_commit, buf.buf);
strbuf_release(&buf);
diff --git a/commit.c b/commit.c
index 97b4327..4bff3cd 100644
--- a/commit.c
+++ b/commit.c
@@ -6,6 +6,7 @@
#include "diff.h"
#include "revision.h"
#include "notes.h"
+#include "gpg-interface.h"
int save_commit_buffer = 1;
@@ -814,6 +815,40 @@ struct commit_list *reduce_heads(struct commit_list *heads)
return result;
}
+static const char gpg_sig_header[] = "gpgsig ";
+static const int gpg_sig_header_len = sizeof(gpg_sig_header) - 1;
+
+static int do_sign_commit(struct strbuf *buf, const char *keyid)
+{
+ struct strbuf sig = STRBUF_INIT;
+ int inspos, copypos;
+
+ /* find the end of the header */
+ inspos = strstr(buf->buf, "\n\n") - buf->buf + 1;
+
+ if (!keyid || !*keyid)
+ keyid = get_signing_key();
+ if (sign_buffer(buf, &sig, keyid)) {
+ strbuf_release(&sig);
+ return -1;
+ }
+
+ for (copypos = 0; sig.buf[copypos]; ) {
+ const char *bol = sig.buf + copypos;
+ const char *eol = strchrnul(bol, '\n');
+ int len = (eol - bol) + !!*eol;
+
+ strbuf_insert(buf, inspos, gpg_sig_header, gpg_sig_header_len);
+ inspos += gpg_sig_header_len;
+ strbuf_insert(buf, inspos, bol, len);
+ inspos += len;
+ copypos += len;
+ }
+ strbuf_release(&sig);
+ return 0;
+}
+
+
static const char commit_utf8_warn[] =
"Warning: commit message does not conform to UTF-8.\n"
"You may want to amend it after fixing the message, or set the config\n"
@@ -821,7 +856,7 @@ static const char commit_utf8_warn[] =
int commit_tree(const char *msg, unsigned char *tree,
struct commit_list *parents, unsigned char *ret,
- const char *author)
+ const char *author, const char *sign_commit)
{
int result;
int encoding_is_utf8;
@@ -864,6 +899,9 @@ int commit_tree(const char *msg, unsigned char *tree,
if (encoding_is_utf8 && !is_utf8(buffer.buf))
fprintf(stderr, commit_utf8_warn);
+ if (sign_commit && do_sign_commit(&buffer, sign_commit))
+ return -1;
+
result = write_sha1_file(buffer.buf, buffer.len, commit_type, ret);
strbuf_release(&buffer);
return result;
diff --git a/commit.h b/commit.h
index 12d100b8..8c2419b 100644
--- a/commit.h
+++ b/commit.h
@@ -175,6 +175,6 @@ struct commit_list *reduce_heads(struct commit_list *heads);
extern int commit_tree(const char *msg, unsigned char *tree,
struct commit_list *parents, unsigned char *ret,
- const char *author);
+ const char *author, const char *sign_commit);
#endif /* COMMIT_H */
diff --git a/notes-cache.c b/notes-cache.c
index 4c8984e..c36a960 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -56,7 +56,7 @@ int notes_cache_write(struct notes_cache *c)
if (write_notes_tree(&c->tree, tree_sha1))
return -1;
- if (commit_tree(c->validity, tree_sha1, NULL, commit_sha1, NULL) < 0)
+ if (commit_tree(c->validity, tree_sha1, NULL, commit_sha1, NULL, NULL) < 0)
return -1;
if (update_ref("update notes cache", c->tree.ref, commit_sha1, NULL,
0, QUIET_ON_ERR) < 0)
diff --git a/notes-merge.c b/notes-merge.c
index e1aaf43..c29c434 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -546,7 +546,7 @@ void create_notes_commit(struct notes_tree *t, struct commit_list *parents,
/* else: t->ref points to nothing, assume root/orphan commit */
}
- if (commit_tree(msg, tree_sha1, parents, result_sha1, NULL))
+ if (commit_tree(msg, tree_sha1, parents, result_sha1, NULL, NULL))
die("Failed to commit notes tree to database");
}
--
1.7.7.388.g3a4b7
^ permalink raw reply related
* [PATCH v3 0/3] Signed-commit
From: Junio C Hamano @ 2011-10-19 0:20 UTC (permalink / raw)
To: git
In-Reply-To: <7vaa9f3pk8.fsf@alter.siamese.dyndns.org>
This is another re-roll of the signed-commit topic.
- The first patch refactors where the current code invokes gpg for
signing and verification of tags;
- The second patch introduces signed commit objects;
- The third patch teaches "git log/show" to show the signature.
The internal API to drive "gpg" has been updated to better suit the
verification side, but I suspect that it needs another round of revamp to
slurp in the diagnostic message "gpg" produces to give us better control
of the formatting of the output. e.g.
$ git show --format="%h %s%n%gpgsign" -s HEAD
might want to say:
a030b7cf log: --show-signature
RSA key ID 96AFE6CB, Good signature from "Junio C Hamano <gitster@pobox.com>"
or something like that, but the series is not there yet.
Junio C Hamano (3):
Split GPG interface into its own helper library
commit: teach --gpg-sign option
log: --show-signature
Makefile | 2 +
builtin/commit-tree.c | 24 ++++++++-
builtin/commit.c | 12 ++++-
builtin/merge.c | 16 +++++-
builtin/tag.c | 76 ++-------------------------
builtin/verify-tag.c | 36 ++-----------
commit.c | 74 ++++++++++++++++++++++++++-
commit.h | 5 ++-
gpg-interface.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
gpg-interface.h | 10 ++++
log-tree.c | 17 ++++++
notes-cache.c | 2 +-
notes-merge.c | 2 +-
revision.c | 2 +
revision.h | 1 +
tag.c | 5 ++
16 files changed, 310 insertions(+), 111 deletions(-)
create mode 100644 gpg-interface.c
create mode 100644 gpg-interface.h
--
1.7.7.388.g3a4b7
^ permalink raw reply
* Re: [PATCH/RFC 3/2 v3] Refactor Git::config_*
From: Jakub Narebski @ 2011-10-19 0:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, Cord Seele, Matthieu Moy, git, Cord Seele
In-Reply-To: <7vmxcxg9wr.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>>> The real problem was here.
>>> ...
>>>> - my $val = command_oneline(@cmd);
>>>> - return undef unless defined $val;
>>>> - return $val eq 'true';
>>>> ...
>>>> - };
>>>> + my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
>>>> + return (defined $val && $val eq 'true');
>>>> }
>>>
>>> Can you spot the difference?
>>
>> Damn.
>
> For people following from the sideline, the difference that bit us was
> that
>
> return $val eq 'true';
>
> yields "" (not undef) for false, but some callers care the distinction
> between undef kind of false and other kinds of false. It is not really the
> fault of the language per-se, but still...
And Git::config* family of commands return 'undef' if key was not found.
So Git::config_bool() can return true, false or undef, and the last part
is important.
>> What I have noticed is that there is slight difference between original
>> Git::config_path and the one after refactoring. In the version before
>> this patch the error catching part of config_path() looks like this:
>> ...
>> return undef;
>> ...
>> while after this patch (and in config()) it looks like this:
>> ...
>> return;
>> ...
>>
>> I am not sure which one is right, but I suspect the latter.
>
> This is Perl---"return;" returns undef, so there is no right or wrong.
Actually this is not true: "return;" returns undef in scalar context,
and empty list in list context. This means that result always evaluates
to false...
> Having said that, I tend to prefer being explicit so that people not so
> familiar with the language do not have to waste time wondering about such
> differences. Especially where it matters, like this case where some
> callers may care about different kinds of falsehood.
>
> That is another reason I tend to hate the kind of "this makes it more
> Perl-ish" changes, as they tend to force readers to spend extra brain
> cells to see what is going on. I'd rather spell things more explicit,
> especially when the distinction matters.
...and that is why "return;" is more Perl-ish, and usually safer.
There might be exceptions where we want to return one-element list
containing single undef element in list context, but I guess they
are exceedingly rare.
> I've already pushed out the fixed one as 6942a3d (libperl-git: refactor
> Git::config_*, 2011-10-18), with this bit:
>
> - ...
> - my $val = command_oneline(@cmd);
> - return undef unless defined $val;
> + # Do not rewrite this as return (defined $val && $val eq 'true')
> + # as some callers do care what kind of falsehood they receive.
> + if (!defined $val) {
> + return undef;
> + } else {
> return $val eq 'true';
> ...
I like mine better... ;-))))
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCH/RFC 3/2 v3] Refactor Git::config_*
From: Junio C Hamano @ 2011-10-18 23:25 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Eric Wong, Cord Seele, Matthieu Moy, git, Cord Seele
In-Reply-To: <201110190009.40470.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
>> Well, this breaks t9001 and I ended up spending an hour and half figuring
>> out why. Admittedly, I was doing something else on the side, so it is not
>> like the whole 90 minutes is the billable hour for reviewing this patch,
>> but as far as the Git project is concerned, I didn't have any other branch
>> checked out in my working tree, so that whole time was what ended up
>> costing.
>
> I'm sorry about that.
No need to be sorry. After all you just re-sent a throw-away patch with
minor tweaks, but unfortunately a tricky Perl script sometimes needs line
by line inspection with fine toothed comb to avoid regression.
>> The real problem was here.
>> ...
>> > - my $val = command_oneline(@cmd);
>> > - return undef unless defined $val;
>> > - return $val eq 'true';
>> > ...
>> > - };
>> > + my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
>> > + return (defined $val && $val eq 'true');
>> > }
>>
>> Can you spot the difference?
>
> Damn.
For people following from the sideline, the difference that bit us was
that
return $val eq 'true';
yields "" (not undef) for false, but some callers care the distinction
between undef kind of false and other kinds of false. It is not really the
fault of the language per-se, but still...
>> This is the reason why I do not particularly like a rewrite for the sake
>> of rewriting.
>
> The goal was to reduce code duplication to _avoid_ errors.
That is exactly why I said I do not like a rewrite for such purpose.
Once the end result of smaller code is done correctly, it would help
avoiding future errors, but people tend to think unconsciously that the
change to reduce code duplication is much safer than adding new code.
Upon receiving such a patch, without knowing that it was not done with
enough attention to detail with fine toothed comb, it is me who ends up
spending nontrivial amount of time fixing the breakage. These "clean-ups"
are not cheap.
> What I have noticed is that there is slight difference between original
> Git::config_path and the one after refactoring. In the version before
> this patch the error catching part of config_path() looks like this:
> ...
> return undef;
> ...
> while after this patch (and in config()) it looks like this:
> ...
> return;
> ...
> I am not sure which one is right, but I suspect the latter.
This is Perl---"return;" returns undef, so there is no right or wrong.
Having said that, I tend to prefer being explicit so that people not so
familiar with the language do not have to waste time wondering about such
differences. Especially where it matters, like this case where some
callers may care about different kinds of falsehood.
That is another reason I tend to hate the kind of "this makes it more
Perl-ish" changes, as they tend to force readers to spend extra brain
cells to see what is going on. I'd rather spell things more explicit,
especially when the distinction matters.
I've already pushed out the fixed one as 6942a3d (libperl-git: refactor
Git::config_*, 2011-10-18), with this bit:
- ...
- my $val = command_oneline(@cmd);
- return undef unless defined $val;
+ # Do not rewrite this as return (defined $val && $val eq 'true')
+ # as some callers do care what kind of falsehood they receive.
+ if (!defined $val) {
+ return undef;
+ } else {
return $val eq 'true';
...
^ permalink raw reply
* [PATCH/RFC 3/2 v3] Refactor Git::config_*
From: Jakub Narebski @ 2011-10-18 22:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, Cord Seele, Matthieu Moy, git, Cord Seele
In-Reply-To: <7vty76f57d.fsf@alter.siamese.dyndns.org>
On Tue, 18 Oct 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> > From: Junio C Hamano <gitster@pobox.com>
> >
> > There is, especially with addition of Git::config_path(), much code
> > repetition in the Git::config_* family of subroutines.
> >
> > Refactor common parts of Git::config(), Git::config_bool(),
> > Git::config_int() and Git::config_path() into _config_common()
> > helper method, reducing code duplication.
> >
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > Jakub Narebski wrote:
> > >
> > > I'll resend amended commit.
> >
> > Here it is.
>
> Well, this breaks t9001 and I ended up spending an hour and half figuring
> out why. Admittedly, I was doing something else on the side, so it is not
> like the whole 90 minutes is the billable hour for reviewing this patch,
> but as far as the Git project is concerned, I didn't have any other branch
> checked out in my working tree, so that whole time was what ended up
> costing.
I'm sorry about that. I have checked t9700-perl-git.sh and
t9100-git-svn-basic.sh that the version before fixup had problems with,
but for some reason I had many spurious test failures, so I have not run
the full test suite (well, it would be enough to run those that require
Git.pm).
Nb. I still have:
Test Summary Report
-------------------
t1402-check-ref-format.sh (Wstat: 256 Tests: 93 Failed: 1)
Failed test: 31
Non-zero exit status: 1
t4034-diff-words.sh (Wstat: 256 Tests: 34 Failed: 2)
Failed tests: 21, 25
Non-zero exit status: 1
> The real problem was here.
>
> > @@ -609,25 +592,10 @@ This currently wraps command('config') so it is not so fast.
> >
> > sub config_bool {
> > my ($self, $var) = _maybe_self(@_);
> > -
> > - try {
> > - my @cmd = ('config', '--bool', '--get', $var);
> > - unshift @cmd, $self if $self;
> > - my $val = command_oneline(@cmd);
> > - return undef unless defined $val;
> > - return $val eq 'true';
> > ...
> > - };
> > + my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
> > + return (defined $val && $val eq 'true');
> > }
>
> Can you spot the difference?
Damn.
I really should have done refactoring from scratch myself, instead of basing
it on "how about that" throwaway patch.
Fixed now in the version below.
> This is the reason why I do not particularly like a rewrite for the sake
> of rewriting.
The goal was to reduce code duplication to _avoid_ errors.
What I have noticed is that there is slight difference between original
Git::config_path and the one after refactoring. In the version before
this patch the error catching part of config_path() looks like this:
} catch Git::Error::Command with {
my $E = shift;
if ($E->value() == 1) {
# Key not found.
return undef;
} else {
throw $E;
}
};
while after this patch (and in config()) it looks like this:
} catch Git::Error::Command with {
my $E = shift;
if ($E->value() == 1) {
# Key not found.
return;
} else {
throw $E;
}
};
I am not sure which one is right, but I suspect the latter.
Cord?
-- >8 ------------ >8 -------------- >8 --
From: Junio C Hamano <gitster@pobox.com>
There is, especially with addition of Git::config_path(), much code
repetition in the Git::config_* family of subroutines.
Refactor common parts of Git::config(), Git::config_bool(),
Git::config_int() and Git::config_path() into _config_common()
helper method, reducing code duplication.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
perl/Git.pm | 73 ++++++++++++++++------------------------------------------
1 files changed, 20 insertions(+), 53 deletions(-)
diff --git a/perl/Git.pm b/perl/Git.pm
index c279bfb..b7035ad 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -577,23 +577,7 @@ This currently wraps command('config') so it is not so fast.
sub config {
my ($self, $var) = _maybe_self(@_);
- try {
- my @cmd = ('config');
- unshift @cmd, $self if $self;
- if (wantarray) {
- return command(@cmd, '--get-all', $var);
- } else {
- return command_oneline(@cmd, '--get', $var);
- }
- } catch Git::Error::Command with {
- my $E = shift;
- if ($E->value() == 1) {
- # Key not found.
- return;
- } else {
- throw $E;
- }
- };
+ return _config_common($self, $var);
}
@@ -610,24 +594,11 @@ This currently wraps command('config') so it is not so fast.
sub config_bool {
my ($self, $var) = _maybe_self(@_);
- try {
- my @cmd = ('config', '--bool', '--get', $var);
- unshift @cmd, $self if $self;
- my $val = command_oneline(@cmd);
- return undef unless defined $val;
- return $val eq 'true';
- } catch Git::Error::Command with {
- my $E = shift;
- if ($E->value() == 1) {
- # Key not found.
- return undef;
- } else {
- throw $E;
- }
- };
+ my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
+ return undef unless defined $val;
+ return $val eq 'true';
}
-
=item config_path ( VARIABLE )
Retrieve the path configuration C<VARIABLE>. The return value
@@ -640,23 +611,7 @@ This currently wraps command('config') so it is not so fast.
sub config_path {
my ($self, $var) = _maybe_self(@_);
- try {
- my @cmd = ('config', '--path');
- unshift @cmd, $self if $self;
- if (wantarray) {
- return command(@cmd, '--get-all', $var);
- } else {
- return command_oneline(@cmd, '--get', $var);
- }
- } catch Git::Error::Command with {
- my $E = shift;
- if ($E->value() == 1) {
- # Key not found.
- return undef;
- } else {
- throw $E;
- }
- };
+ return _config_common($self, $var, {'kind' => '--path'});
}
=item config_int ( VARIABLE )
@@ -674,15 +629,27 @@ This currently wraps command('config') so it is not so fast.
sub config_int {
my ($self, $var) = _maybe_self(@_);
+ return scalar _config_common($self, $var, {'kind' => '--int'});
+}
+
+# Common subroutine to implement bulk of what the config* family of methods
+# do. This curently wraps command('config') so it is not so fast.
+sub _config_common {
+ my ($self, $var, $opts) = @_;
+
try {
- my @cmd = ('config', '--int', '--get', $var);
+ my @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());
unshift @cmd, $self if $self;
- return command_oneline(@cmd);
+ if (wantarray) {
+ return command(@cmd, '--get-all', $var);
+ } else {
+ return command_oneline(@cmd, '--get', $var);
+ }
} catch Git::Error::Command with {
my $E = shift;
if ($E->value() == 1) {
# Key not found.
- return undef;
+ return;
} else {
throw $E;
}
--
1.7.6
^ permalink raw reply related
* Git --reference bug(?)
From: Andrea Gelmini @ 2011-10-18 22:04 UTC (permalink / raw)
To: mhagger; +Cc: gitster, git
Hi Michael,
and thanks a lot for your time on Git.
I have a problem with latest Git tree:
git clone git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
/tmp/3.1 --reference /home/gelma/dev/kernel/linus/
Cloning into /tmp/3.1...
fatal: Reference has invalid format: 'refs/tags/3.1.1.1^{}'
fatal: The remote end hung up unexpectedly
It works with Ubuntu Git version 1.7.4.1, of course.
Well, bisecting I've got this:
dce4bab6567de7c458b334e029e3dedcab5f2648 is the first bad commit
commit dce4bab6567de7c458b334e029e3dedcab5f2648
Author: Michael Haggerty <mhagger@alum.mit.edu>
Date: Thu Sep 15 23:10:43 2011 +0200
add_ref(): verify that the refname is formatted correctly
In add_ref(), verify that the refname is formatted correctly before
adding it to the ref_list. Here we have to allow refname components
that start with ".", since (for example) the remote protocol uses
synthetic reference name ".have". So add a new REFNAME_DOT_COMPONENT
flag that can be passed to check_refname_format() to allow leading
dots.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
:100644 100644 096b42c5e993193dd83a02128be4b90ebc59edd1
832a52f7818369bca969d49317718714a5bcabac M refs.c
:100644 100644 b0da5fc95dff025a8dd5c1f299ee25efc6141e81
d5ac133336dc0da45cd916207d12a5e0e4237ae3 M refs.h
bisect run success
git bisect log
git bisect start
# bad: [85bb77ed056057b727ba8dc7965fbfcde987d189] Merge branch
'master' of git://git.kernel.org/pub/scm/git/git
git bisect bad 85bb77ed056057b727ba8dc7965fbfcde987d189
# good: [9971d6d52c5afeb8ba60ae6ddcffb34af23eeadd] Git 1.7.4.1
git bisect good 9971d6d52c5afeb8ba60ae6ddcffb34af23eeadd
# good: [456a4c08b8d8ddefda939014c15877ace3e3f499] Merge branch
'jk/diff-not-so-quick'
git bisect good 456a4c08b8d8ddefda939014c15877ace3e3f499
# good: [18dce8d4226c56b10d2b783b476008117d60a23e] Merge branch
'master' of git://git.kernel.org/pub/scm/git/git
git bisect good 18dce8d4226c56b10d2b783b476008117d60a23e
# good: [821b315ebebd5371abd9124478261064b36a6592] Merge branch
'da/make-auto-header-dependencies'
git bisect good 821b315ebebd5371abd9124478261064b36a6592
# bad: [e579a5d398744e8182decff1329c468d59e6974c] Merge branch
'mh/maint-notes-merge-pathbuf-fix'
git bisect bad e579a5d398744e8182decff1329c468d59e6974c
# good: [17e2b114a8f9d66d7c9263755f89cc503c94c38c] Merge branch
'jn/gitweb-highlite-sanitise'
git bisect good 17e2b114a8f9d66d7c9263755f89cc503c94c38c
# good: [5fbdb9c2e8cc7226d9a9e7de5ad09ac5f0a0b906] Merge branch
'jm/mergetool-pathspec'
git bisect good 5fbdb9c2e8cc7226d9a9e7de5ad09ac5f0a0b906
# good: [f989fea0e0b47873de62a355f4766f03a88fb01b] resolve_ref(): also
treat a too-long SHA1 as invalid
git bisect good f989fea0e0b47873de62a355f4766f03a88fb01b
# bad: [9bd500048d467791902b1a5e8c22165325952fde] Merge branch
'mh/check-ref-format-3'
git bisect bad 9bd500048d467791902b1a5e8c22165325952fde
# good: [ce40979cf83c4c92421f9dd56cc4eabb67c85f29] Store the submodule
name in struct cached_refs
git bisect good ce40979cf83c4c92421f9dd56cc4eabb67c85f29
# good: [11fa509957025cc30c063d75014b701dd9ae235d] Merge branch
'mh/iterate-refs'
git bisect good 11fa509957025cc30c063d75014b701dd9ae235d
# bad: [dce4bab6567de7c458b334e029e3dedcab5f2648] add_ref(): verify
that the refname is formatted correctly
git bisect bad dce4bab6567de7c458b334e029e3dedcab5f2648
# good: [7cb368421f62318f2c0f0e19a83ca34c201aebaa] resolve_ref():
expand documentation
git bisect good 7cb368421f62318f2c0f0e19a83ca34c201aebaa
If I revert it, everything's ok.
Thanks a lot,
Andrea Gelmini
^ permalink raw reply
* Re: [PATCH v4 2/2] push: teach --recurse-submodules the on-demand option
From: Jens Lehmann @ 2011-10-18 20:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Heiko Voigt, Fredrik Gustafsson, git
In-Reply-To: <7vr52bjljd.fsf@alter.siamese.dyndns.org>
Am 18.10.2011 00:33, schrieb Junio C Hamano:
> I would personally want to put a freeze on "recursively do anything to
> submodule" topic (including but not limited to "checkout") for now, until
> we know how we would want to support "floating submodule" model. For
> existing code in-flight, I would like to see us at least have a warm and
> fuzzy feeling that we know which part of the code such a support would
> need to undo and how the update would look like before moving forward.
Makes sense.
> There are two camps that use submodules in their large-ish projects.
>
> One is mostly happy with the traditional "submodule trees checked out must
> match what the superproject says, otherwise you have local changes and the
> build product cannot be called to have emerged from that particular
> superproject commit" model. Let's call this "exact submodules" model.
>
> The other prefers "submodule trees checked out are whatever submodule
> commits that happen to sit at the tips of the designated branches the
> superproject wants to use" model. The superproject tree does not exactly
> know or care what commit to use from each of its submodules, and I would
> imagine that it may be more convenient for developers. They do not have to
> care the entire build product while they commit---only the integration
> process that could be separate and perhaps automated needs to know.
>
> We haven't given any explicit support to the latter "floating submodules"
> model so far. There may be easy workarounds to many of the potential
> issues, (e.g. at "git diff/status" level, there may be some configuration
> variables to tell the tools to ignore differences between the commit the
> superproject records for the submodule path and the HEAD in the
> submodule), but with recent work on submodules such as "allow pushing
> superproject only after submodule commits are pushed out", I am afraid
> that we seem to be piling random new things with the assumption that we
> would never support anything but "exact submodules" model.
It's not about never supporting anything else, but right now we are
scratching our own itch ;-)
> Continuing the
> development that way would require retrofitting support for "floating
> submodules" model to largely undo the unwarranted assumptions existing
> code makes. That is the reason why I would like to see people think about
> the need to support the other "floating submodules" model, before making
> the existing mess even worse.
If you configure diff.ignoreSubmodules=all and fetch.recurseSubmodules=false
and write a script fetching and checking out the branch(es) of your choice
in the submodule(s) you run each time you want to update the branch tip
there, you should be almost there with current Git. But yes, we could do
better.
> The very first step for floating submodules support would be relatively
> simple. We could declare that an entry in the .gitmodules file in the
> superproject can optionally specify which branch needs to be checked out
> with something like:
>
> [submodule "libfoo"]
> branch = master
> path = include/foo
> url = git://foo.com/git/lib.git
>
> and when such an entry is defined, a command at the superproject level
> would largely ignore what is at include/foo in the tree object recorded in
> the superproject commit and in the index. When we show "git status" in the
> superproject, instead of using the commit bound to the superproject, we
> would use include/foo/.git/HEAD as the basis for detecting "local" changes
> to the submodule.
Yup. And the presence of the "branch" config could tell "git submodule
update" to fetch and advance that branch to the tip every time it is run.
And it could tell the diff machinery (which is also used by status) to
ignore the differences between a submodule's HEAD and the SHA-1 in the
superproject (while still allowing to silence the presence of untracked
and/or modified files by using the diff.ignoreSubmodules option) and
fetch would just stop doing any on-demand action for such submodules.
Anything I missed?
> We could even declare that the gitlink for such a
> submodule should record 0{40} SHA-1 in the superproject, but I do not
> think that is necessary.
Me neither, e.g. the SHA-1 which was the submodules HEAD when it was added
should do nicely. And that would avoid referencing a non-existing commit
in case you later want to turn a floating submodule into an exact one.
^ permalink raw reply
* Re: [PATCH 2/2] daemon: report permission denied error to clients
From: Clemens Buchacher @ 2011-10-18 20:41 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vhb37l4ag.fsf@alter.siamese.dyndns.org>
On Mon, Oct 17, 2011 at 02:03:03PM -0700, Junio C Hamano wrote:
>
> I personally think this is going a bit too far, even for "informative"
> option, by allowing to fish for possible list of usernames. It would make
> it a tougher sell to later default to "informative", I am afraid.
I guess if permission is denied for access over git://, then nobody
can use the repository. So it's clearly a server side issue.
This change probably makes more sense for local access and over
ssh. I already have a similar patch brewing for that.
Clemens
^ permalink raw reply
* Re: [PATCH] use test number as port number
From: Clemens Buchacher @ 2011-10-18 20:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vobxfl4kj.fsf@alter.siamese.dyndns.org>
On Mon, Oct 17, 2011 at 01:57:00PM -0700, Junio C Hamano wrote:
>
> I've fixed and queued the previous one as aa0b028 (daemon: add tests,
> 2011-10-17); does that look good enough?
Yep, perfect. Thanks.
^ permalink raw reply
* Re: [GUILT] handle branches with slashes in guilt-graph
From: Andreas Schwab @ 2011-10-18 19:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Per Cederqvist, Jeff Sipek, git, ceder
In-Reply-To: <7v39eqglle.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Andreas Schwab <schwab@linux-m68k.org> writes:
>
>> Per Cederqvist <cederp@opera.com> writes:
>>
>>> Avoid sed errors when the branch name contains a slash.
>>>
>>> Signed-off-by: Per Cederqvist <cederp@opera.com>
>>>
>>> --- /usr/bin/guilt-graph~ 2011-01-25 20:15:50.000000000 +0100
>>> +++ /usr/bin/guilt-graph 2011-10-18 12:30:31.000000000 +0200
>>> @@ -37,9 +37,10 @@ disp "digraph G {"
>>>
>>> current="$top"
>>>
>>> +safebranch=`echo "$branch"|sed 's%/%\\\\/%g'`
>>> while [ "$current" != "$base" ]; do
>>> pname=`git show-ref | sed -n -e "
>>> -/^$current refs\/patches\/$branch/ {
>>> +/^$current refs\/patches\/$safebranch/ {
>>
>> Alternatively, you could change the delimiter to `,':
>>
>> \,^$current refs/patches/$branch, {
>
> Isn't a comma still valid character in a branch name?
I suggested the comma because it is already used by the next sed
command, so that won't be a regression.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: Problems after deleting submodule
From: Jens Lehmann @ 2011-10-18 19:56 UTC (permalink / raw)
To: Howard Miller; +Cc: git
In-Reply-To: <CAHVO_90UN_nNDbqxM2TkUjo_qfVhLgjUJoZTmYi6rsLnRMOUFg@mail.gmail.com>
Am 18.10.2011 13:54, schrieb Howard Miller:
> I included a submodule in my project then decided I didn't like
> submodules and deleted it again. I followed the advice of delting
> .gitmodules, the bit from .git/config and then git rm'ing the
> submodule. Seemed to work. I then copied files with the same directory
> name into where the submodule was. However, I can't add them.
>
> Doing git add /path/to/old/submodule - does nothing, files are not
> staged, no error messages no nothing.
> If I try to git rm /path/to/old/submodule - it just says 'did not
> match any files'.
>
> It simply does not seem to want to add anything to the old submodule
> location. I had a grep around and could not see any obvious references
> in the repo.
>
> I'm a bit stuck... any suggestions for things I can try much appreciated.
Looks like the gitlink entry of the submodule is still there. I assume
git ls-files --stage | grep 160000
still shows the submodule? That would make it impossible to add anything
in the former submodule directory.
When I delete .gitmodules and the .git/config entry and do a
git rm sub/
I get
fatal: pathspec 'sub/' did not match any files
If I omit the trailing slash it is:
fatal: git rm: 'sub': Is a directory
I have to do a
rm -rf sub
followed by
git rm sub
to get everything cleaned up.
Does that help you?
^ permalink raw reply
* Re: [PATCH/RFC 3/2 (fixed)] Refactor Git::config_*
From: Junio C Hamano @ 2011-10-18 19:52 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Eric Wong, Cord Seele, Matthieu Moy, git, Cord Seele
In-Reply-To: <201110181147.02397.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> From: Junio C Hamano <gitster@pobox.com>
>
> There is, especially with addition of Git::config_path(), much code
> repetition in the Git::config_* family of subroutines.
>
> Refactor common parts of Git::config(), Git::config_bool(),
> Git::config_int() and Git::config_path() into _config_common()
> helper method, reducing code duplication.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> Jakub Narebski wrote:
>>
>> I'll resend amended commit.
>
> Here it is.
Well, this breaks t9001 and I ended up spending an hour and half figuring
out why. Admittedly, I was doing something else on the side, so it is not
like the whole 90 minutes is the billable hour for reviewing this patch,
but as far as the Git project is concerned, I didn't have any other branch
checked out in my working tree, so that whole time was what ended up
costing.
The real problem was here.
> @@ -609,25 +592,10 @@ This currently wraps command('config') so it is not so fast.
>
> sub config_bool {
> my ($self, $var) = _maybe_self(@_);
> -
> - try {
> - my @cmd = ('config', '--bool', '--get', $var);
> - unshift @cmd, $self if $self;
> - my $val = command_oneline(@cmd);
> - return undef unless defined $val;
> - return $val eq 'true';
> ...
> - };
> + my $val = scalar _config_common($self, $var, {'kind' => '--bool'});
> + return (defined $val && $val eq 'true');
> }
Can you spot the difference?
This is the reason why I do not particularly like a rewrite for the sake
of rewriting.
^ permalink raw reply
* Re: [GUILT] handle branches with slashes in guilt-graph
From: Junio C Hamano @ 2011-10-18 19:15 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Per Cederqvist, Jeff Sipek, git, ceder
In-Reply-To: <7v39eqglle.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> Alternatively, you could change the delimiter to `,':
>>
>> \,^$current refs/patches/$branch, {
>
> Isn't a comma still valid character in a branch name?
>
> The vertical var | is available, though ;-)
Nah, sorry, '|' is not.
^ permalink raw reply
* Re: [GUILT] handle branches with slashes in guilt-graph
From: Junio C Hamano @ 2011-10-18 19:12 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Per Cederqvist, Jeff Sipek, git, ceder
In-Reply-To: <m2r52adu65.fsf@igel.home>
Andreas Schwab <schwab@linux-m68k.org> writes:
> Per Cederqvist <cederp@opera.com> writes:
>
>> Avoid sed errors when the branch name contains a slash.
>>
>> Signed-off-by: Per Cederqvist <cederp@opera.com>
>>
>> --- /usr/bin/guilt-graph~ 2011-01-25 20:15:50.000000000 +0100
>> +++ /usr/bin/guilt-graph 2011-10-18 12:30:31.000000000 +0200
>> @@ -37,9 +37,10 @@ disp "digraph G {"
>>
>> current="$top"
>>
>> +safebranch=`echo "$branch"|sed 's%/%\\\\/%g'`
>> while [ "$current" != "$base" ]; do
>> pname=`git show-ref | sed -n -e "
>> -/^$current refs\/patches\/$branch/ {
>> +/^$current refs\/patches\/$safebranch/ {
>
> Alternatively, you could change the delimiter to `,':
>
> \,^$current refs/patches/$branch, {
Isn't a comma still valid character in a branch name?
The vertical var | is available, though ;-)
^ permalink raw reply
* Re: [GUILT] handle branches with slashes in guilt-graph
From: Andreas Schwab @ 2011-10-18 18:35 UTC (permalink / raw)
To: Per Cederqvist; +Cc: Jeff Sipek, git, ceder
In-Reply-To: <4E9D57BB.2030707@opera.com>
Per Cederqvist <cederp@opera.com> writes:
> Avoid sed errors when the branch name contains a slash.
>
> Signed-off-by: Per Cederqvist <cederp@opera.com>
>
> --- /usr/bin/guilt-graph~ 2011-01-25 20:15:50.000000000 +0100
> +++ /usr/bin/guilt-graph 2011-10-18 12:30:31.000000000 +0200
> @@ -37,9 +37,10 @@ disp "digraph G {"
>
> current="$top"
>
> +safebranch=`echo "$branch"|sed 's%/%\\\\/%g'`
> while [ "$current" != "$base" ]; do
> pname=`git show-ref | sed -n -e "
> -/^$current refs\/patches\/$branch/ {
> +/^$current refs\/patches\/$safebranch/ {
Alternatively, you could change the delimiter to `,':
\,^$current refs/patches/$branch, {
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* [PATCH] gitweb: Refactor diff body line classification
From: Jakub Narebski @ 2011-10-18 18:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kato Kazuyoshi, git
In-Reply-To: <7v62jnl3ef.fsf@alter.siamese.dyndns.org>
Simplify classification of diff line body in format_diff_line(),
replacing two if-elsif chains (one for ordinary diff and one for
combined diff of a merge commit) with regexp match. Refactor this
code into diff_line_class() function.
While at it:
* Fix an artifact in that $diff_class included leading space to be
able to compose classes like this "class=\"diff$diff_class\"', even
when $diff_class was an empty string. This made code unnecessary
ugly: $diff_class is now just class name or an empty string.
* Introduce "ctx" class for context lines ($diff_class was set to ""
in this case before this commit).
Idea and initial code by Junio C Hamano, polish and testing by Jakub
Narebski. Inspired by patch adding side-by-side diff by Kato Kazuyoshi,
which required $diff_class to be name of class without extra space.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>> Kato Kazuyoshi wrote:
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index 85d64b2..095adda 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -2235,28 +2235,30 @@ sub format_diff_line {
>>> # combined diff
>>> my $prefix = substr($line, 0, scalar @{$from->{'href'}});
>>> if ($line =~ m/^\@{3}/) {
>>> - $diff_class = " chunk_header";
>>> + $diff_class = "chunk_header";
>>> } elsif ($line =~ m/^\\/) {
>>> - $diff_class = " incomplete";
>>> + $diff_class = "incomplete";
>>> } elsif ($prefix =~ tr/+/+/) {
>>> - $diff_class = " add";
>>> + $diff_class = "add";
>>> } elsif ($prefix =~ tr/-/-/) {
>>> - $diff_class = " rem";
>>> + $diff_class = "rem";
>>> }
>>
>> Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would
>> be hard to replace without given ... when, I think.
>
> I wasn't reading the existing context line, but now that you mention it,
> they are not just ugly but are borderline of being incorrect (e.g. it does
> not catch a broken hunk-header that begins with "@@@@" for a two-way
> merge).
>
> Assuming that $from->{'href'} has all the parents (i.e. 2 for two-way
> merge), shouldn't the code be more like this?
>
> # combined diff
> my $num_sign = @{$from->{'href'}} + 1;
> my @cc_classifier = (
> ["\@{$num_sign} ", "chunk_header"],
> ['\\', "incomplete"],
> [" {$num_sign}", ""],
> ["[+ ]{$num_sign}", "add"],
> ["[- ]{$num_sign}", "rem"],
> );
> for my $cls (@cc_classifier) {
> if ($line =~ /^$cls->[0]$/) {
> $diff_class = $cls->[1];
> last;
> }
> }
>
> Also don't we want to use "context" or something for the css class for the
> context lines, instead of assuming that we won't want to paint it in any
> special color?
How about this?
Kato Kazuyoshi, this should replace your
[PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
gitweb/gitweb.perl | 67 ++++++++++++++++++++++++++++-----------------------
1 files changed, 37 insertions(+), 30 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b0eaad7..b3284d4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2247,40 +2247,47 @@ sub format_diff_cc_simplified {
return $result;
}
+sub diff_line_class {
+ my ($line, $from, $to) = @_;
+
+ # ordinary diff
+ my $num_sign = 1;
+ # combined diff
+ if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
+ $num_sign = scalar @{$from->{'href'}};
+ }
+
+ my @diff_line_classifier = (
+ { regexp => qr/^\@\@{$num_sign} /, class => "chunk_header"},
+ { regexp => qr/^\\/, class => "incomplete" },
+ { regexp => qr/^ {$num_sign}/, class => "ctx" },
+ # classifier for context must come before classifier add/rem,
+ # or we would have to use more complicated regexp, for example
+ # qr/(?= {0,$m}\+)[+ ]{$num_sign}/, where $m = $num_sign - 1;
+ { regexp => qr/^[+ ]{$num_sign}/, class => "add" },
+ { regexp => qr/^[- ]{$num_sign}/, class => "rem" },
+ );
+ for my $clsfy (@diff_line_classifier) {
+ return $clsfy->{'class'}
+ if ($line =~ $clsfy->{'regexp'});
+ }
+
+ # fallback
+ return "";
+}
+
# format patch (diff) line (not to be used for diff headers)
sub format_diff_line {
my $line = shift;
my ($from, $to) = @_;
- my $diff_class = "";
- chomp $line;
+ my $diff_class = diff_line_class($line, $from, $to);
+ my $diff_classes = "diff";
+ $diff_classes .= " $diff_class" if ($diff_class);
- if ($from && $to && ref($from->{'href'}) eq "ARRAY") {
- # combined diff
- my $prefix = substr($line, 0, scalar @{$from->{'href'}});
- if ($line =~ m/^\@{3}/) {
- $diff_class = " chunk_header";
- } elsif ($line =~ m/^\\/) {
- $diff_class = " incomplete";
- } elsif ($prefix =~ tr/+/+/) {
- $diff_class = " add";
- } elsif ($prefix =~ tr/-/-/) {
- $diff_class = " rem";
- }
- } else {
- # assume ordinary diff
- my $char = substr($line, 0, 1);
- if ($char eq '+') {
- $diff_class = " add";
- } elsif ($char eq '-') {
- $diff_class = " rem";
- } elsif ($char eq '@') {
- $diff_class = " chunk_header";
- } elsif ($char eq "\\") {
- $diff_class = " incomplete";
- }
- }
+ chomp $line;
$line = untabify($line);
+
if ($from && $to && $line =~ m/^\@{2} /) {
my ($from_text, $from_start, $from_lines, $to_text, $to_start, $to_lines, $section) =
$line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
@@ -2298,7 +2305,7 @@ sub format_diff_line {
}
$line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
"<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
- return "<div class=\"diff$diff_class\">$line</div>\n";
+ return "<div class=\"$diff_classes\">$line</div>\n";
} elsif ($from && $to && $line =~ m/^\@{3}/) {
my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
@@ -2331,9 +2338,9 @@ sub format_diff_line {
}
$line .= " $prefix</span>" .
"<span class=\"section\">" . esc_html($section, -nbsp=>1) . "</span>";
- return "<div class=\"diff$diff_class\">$line</div>\n";
+ return "<div class=\"$diff_classes\">$line</div>\n";
}
- return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1) . "</div>\n";
+ return "<div class=\"$diff_classes\">" . esc_html($line, -nbsp=>1) . "</div>\n";
}
# Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
--
1.7.6
^ permalink raw reply related
* Re: [PATCH] inet_ntop.c: Work around GCC 4.6's detection of uninitialized variables
From: Sebastian Schuberth @ 2011-10-18 18:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, mschub
In-Reply-To: <7vehyagq82.fsf@alter.siamese.dyndns.org>
GCC 4.6 claims that
error: 'best.len' may be used uninitialized in this function
so silence that warning which is treated as an error by also initializing
the "len" members of the struct.
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
---
compat/inet_ntop.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/compat/inet_ntop.c b/compat/inet_ntop.c
index ea249c6..9e9f6fb 100644
--- a/compat/inet_ntop.c
+++ b/compat/inet_ntop.c
@@ -98,7 +98,8 @@ inet_ntop6(const u_char *src, char *dst, size_t size)
for (i = 0; i < NS_IN6ADDRSZ; i++)
words[i / 2] |= (src[i] << ((1 - (i % 2)) << 3));
best.base = -1;
- cur.base = -1;
+ best.len = 0;
+ cur = best;
for (i = 0; i < (NS_IN6ADDRSZ / NS_INT16SZ); i++) {
if (words[i] == 0) {
if (cur.base == -1)
--
1.7.7.msysgit.1
^ permalink raw reply related
* Re: What's cooking in git.git (Oct 2011, #06; Tue, 18)
From: Junio C Hamano @ 2011-10-18 18:09 UTC (permalink / raw)
To: Phil Hord; +Cc: git
In-Reply-To: <7vmxcygs1m.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> On Tue, Oct 18, 2011 at 3:50 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> What's cooking in git.git (Oct 2011, #06; Tue, 18)
>>> --------------------------------------------------
>> [...]
>>> * ph/transport-with-gitfile (2011-10-11) 5 commits
>>> (merged to 'next' on 2011-10-12 at 6d58417)
>>> + Fix is_gitfile() for files too small or larger than PATH_MAX to be a gitfile
>>> (merged to 'next' on 2011-10-06 at 891b8b6)
>>> + Add test showing git-fetch groks gitfiles
>>> + Teach transport about the gitfile mechanism
>>> + Learn to handle gitfiles in enter_repo
>>> + enter_repo: do not modify input
>>>
>>> Will merge to 'master' in the fifth wave.
>>
>> Do you want a re-roll of this with your is_bundle() changes added in?
>> I do like them better.
>
> Hmm, you may be right. I'll try to queue an update on top of the series
> and see what happens.
Actually there is nothing that needs re-rolling. We can just make the
jc/unseekable-bundle topic graduate at the same time as this one, and let
the merge remove the is_gitfile() helper that becomes unnecessary.
^ 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