* [PATCH 2/5] do not override receive-pack errors
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>
Receive runs rev-list --verify-objects in order to detect missing
objects. However, such errors are ignored and overridden later.
Instead, consequently ignore all update commands for which an error has
already been detected.
Some tests in t5504 are obsoleted by this change, because invalid
objects are detected even if fsck is not enabled. Instead, they now test
for different error messages depending on whether or not fsck is turned
on. A better fix would be to force a corruption that will be detected by
fsck but not by rev-list.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
builtin/receive-pack.c | 24 +++++++++++++++++-------
t/t5504-fetch-receive-strict.sh | 22 ++++++++++++++++++----
2 files changed, 35 insertions(+), 11 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index fa7448b..0afb8b2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -642,8 +642,10 @@ static void check_aliased_updates(struct command *commands)
}
sort_string_list(&ref_list);
- for (cmd = commands; cmd; cmd = cmd->next)
- check_aliased_update(cmd, &ref_list);
+ for (cmd = commands; cmd; cmd = cmd->next) {
+ if (!cmd->error_string)
+ check_aliased_update(cmd, &ref_list);
+ }
string_list_clear(&ref_list, 0);
}
@@ -707,8 +709,10 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
set_connectivity_errors(commands);
if (run_receive_hook(commands, pre_receive_hook, 0)) {
- for (cmd = commands; cmd; cmd = cmd->next)
- cmd->error_string = "pre-receive hook declined";
+ for (cmd = commands; cmd; cmd = cmd->next) {
+ if (!cmd->error_string)
+ cmd->error_string = "pre-receive hook declined";
+ }
return;
}
@@ -717,9 +721,15 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
free(head_name_to_free);
head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
- for (cmd = commands; cmd; cmd = cmd->next)
- if (!cmd->skip_update)
- cmd->error_string = update(cmd);
+ for (cmd = commands; cmd; cmd = cmd->next) {
+ if (cmd->error_string)
+ continue;
+
+ if (cmd->skip_update)
+ continue;
+
+ cmd->error_string = update(cmd);
+ }
}
static struct command *read_head_info(void)
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 8341fc4..35ec294 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -58,6 +58,11 @@ test_expect_success 'fetch with transfer.fsckobjects' '
)
'
+cat >exp <<EOF
+To dst
+! refs/heads/master:refs/heads/test [remote rejected] (missing necessary objects)
+EOF
+
test_expect_success 'push without strict' '
rm -rf dst &&
git init dst &&
@@ -66,7 +71,8 @@ test_expect_success 'push without strict' '
git config fetch.fsckobjects false &&
git config transfer.fsckobjects false
) &&
- git push dst master:refs/heads/test
+ test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+ test_cmp exp act
'
test_expect_success 'push with !receive.fsckobjects' '
@@ -77,9 +83,15 @@ test_expect_success 'push with !receive.fsckobjects' '
git config receive.fsckobjects false &&
git config transfer.fsckobjects true
) &&
- git push dst master:refs/heads/test
+ test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+ test_cmp exp act
'
+cat >exp <<EOF
+To dst
+! refs/heads/master:refs/heads/test [remote rejected] (n/a (unpacker error))
+EOF
+
test_expect_success 'push with receive.fsckobjects' '
rm -rf dst &&
git init dst &&
@@ -88,7 +100,8 @@ test_expect_success 'push with receive.fsckobjects' '
git config receive.fsckobjects true &&
git config transfer.fsckobjects false
) &&
- test_must_fail git push dst master:refs/heads/test
+ test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+ test_cmp exp act
'
test_expect_success 'push with transfer.fsckobjects' '
@@ -98,7 +111,8 @@ test_expect_success 'push with transfer.fsckobjects' '
cd dst &&
git config transfer.fsckobjects true
) &&
- test_must_fail git push dst master:refs/heads/test
+ test_must_fail git push --porcelain dst master:refs/heads/test >act &&
+ test_cmp exp act
'
test_done
--
1.7.9
^ permalink raw reply related
* [PATCH 5/5] push/fetch/clone --no-progress suppresses progress output
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>
By default, progress output is disabled if stderr is not a terminal.
The --progress option can be used to force progress output anyways.
Conversely, --no-progress does not force progress output. In particular,
if stderr is a terminal, progress output is enabled.
This is unintuitive. Change --no-progress to force output off.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
builtin/clone.c | 6 +++---
builtin/fetch-pack.c | 2 +-
builtin/fetch.c | 4 ++--
builtin/push.c | 4 ++--
builtin/send-pack.c | 12 +++++++-----
t/t5523-push-upstream.sh | 3 ++-
transport.c | 6 +++++-
7 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index c62d4b5..09dbb09 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -45,7 +45,7 @@ static char *option_branch = NULL;
static const char *real_git_dir;
static char *option_upload_pack = "git-upload-pack";
static int option_verbosity;
-static int option_progress;
+static int option_progress = -1;
static struct string_list option_config;
static struct string_list option_reference;
@@ -60,8 +60,8 @@ static int opt_parse_reference(const struct option *opt, const char *arg, int un
static struct option builtin_clone_options[] = {
OPT__VERBOSITY(&option_verbosity),
- OPT_BOOLEAN(0, "progress", &option_progress,
- "force progress reporting"),
+ OPT_BOOL(0, "progress", &option_progress,
+ "force progress reporting"),
OPT_BOOLEAN('n', "no-checkout", &option_no_checkout,
"don't create a checkout"),
OPT_BOOLEAN(0, "bare", &option_bare, "create a bare repository"),
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 6207ecd..a4d3e90 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -736,7 +736,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
}
else {
*av++ = "unpack-objects";
- if (args.quiet)
+ if (args.quiet || args.no_progress)
*av++ = "-q";
}
if (*hdr_arg)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index ab18633..65f5f9b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -30,7 +30,7 @@ enum {
};
static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;
-static int progress, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
static int tags = TAGS_DEFAULT;
static const char *depth;
static const char *upload_pack;
@@ -78,7 +78,7 @@ static struct option builtin_fetch_options[] = {
OPT_BOOLEAN('k', "keep", &keep, "keep downloaded pack"),
OPT_BOOLEAN('u', "update-head-ok", &update_head_ok,
"allow updating of HEAD ref"),
- OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+ OPT_BOOL(0, "progress", &progress, "force progress reporting"),
OPT_STRING(0, "depth", &depth, "depth",
"deepen history of shallow clone"),
{ OPTION_STRING, 0, "submodule-prefix", &submodule_prefix, "dir",
diff --git a/builtin/push.c b/builtin/push.c
index 35cce53..6c373cf 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -19,7 +19,7 @@ static int thin;
static int deleterefs;
static const char *receivepack;
static int verbosity;
-static int progress;
+static int progress = -1;
static const char **refspec;
static int refspec_nr;
@@ -260,7 +260,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_STRING( 0 , "exec", &receivepack, "receive-pack", "receive pack program"),
OPT_BIT('u', "set-upstream", &flags, "set upstream for git pull/status",
TRANSPORT_PUSH_SET_UPSTREAM),
- OPT_BOOLEAN(0, "progress", &progress, "force progress reporting"),
+ OPT_BOOL(0, "progress", &progress, "force progress reporting"),
OPT_END()
};
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 7d514ca..8fc9789 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -58,7 +58,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
argv[i++] = "--thin";
if (args->use_ofs_delta)
argv[i++] = "--delta-base-offset";
- if (args->quiet)
+ if (args->quiet || !args->progress)
argv[i++] = "-q";
if (args->progress)
argv[i++] = "--progress";
@@ -250,6 +250,7 @@ int send_pack(struct send_pack_args *args,
int allow_deleting_refs = 0;
int status_report = 0;
int use_sideband = 0;
+ int quiet_supported = 0;
unsigned cmds_sent = 0;
int ret;
struct async demux;
@@ -263,8 +264,8 @@ int send_pack(struct send_pack_args *args,
args->use_ofs_delta = 1;
if (server_supports("side-band-64k"))
use_sideband = 1;
- if (!server_supports("quiet"))
- args->quiet = 0;
+ if (server_supports("quiet"))
+ quiet_supported = 1;
if (server_supports("verify-refs"))
args->verify_refs = 1;
@@ -304,13 +305,14 @@ int send_pack(struct send_pack_args *args,
} else {
char *old_hex = sha1_to_hex(ref->old_sha1);
char *new_hex = sha1_to_hex(ref->new_sha1);
+ int quiet = quiet_supported && (args->quiet || !args->progress);
- if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
+ if (!cmds_sent && (status_report || use_sideband || quiet || args->verify_refs)) {
packet_buf_write(&req_buf, "%s %s %s%c%s%s%s%s",
old_hex, new_hex, ref->name, 0,
status_report ? " report-status" : "",
use_sideband ? " side-band-64k" : "",
- args->quiet ? " quiet" : "",
+ quiet ? " quiet" : "",
args->verify_refs ? " verify-refs" : "");
}
else
diff --git a/t/t5523-push-upstream.sh b/t/t5523-push-upstream.sh
index 9ee52cf..3683df1 100755
--- a/t/t5523-push-upstream.sh
+++ b/t/t5523-push-upstream.sh
@@ -101,10 +101,11 @@ test_expect_success TTY 'push -q suppresses progress' '
! grep "Writing objects" err
'
-test_expect_failure TTY 'push --no-progress suppresses progress' '
+test_expect_success TTY 'push --no-progress suppresses progress' '
ensure_fresh_upstream &&
test_terminal git push -u --no-progress upstream master >out 2>err &&
+ ! grep "Unpacking objects" err &&
! grep "Writing objects" err
'
diff --git a/transport.c b/transport.c
index cac0c06..6074ee9 100644
--- a/transport.c
+++ b/transport.c
@@ -994,10 +994,14 @@ void transport_set_verbosity(struct transport *transport, int verbosity,
* when a rule is satisfied):
*
* 1. Report progress, if force_progress is 1 (ie. --progress).
+ * 2. Don't report progress, if force_progress is 0 (ie. --no-progress).
* 2. Don't report progress, if verbosity < 0 (ie. -q/--quiet ).
* 3. Report progress if isatty(2) is 1.
**/
- transport->progress = force_progress || (verbosity >= 0 && isatty(2));
+ if (force_progress >= 0)
+ transport->progress = !!force_progress;
+ else
+ transport->progress = verbosity >= 0 && isatty(2);
}
int transport_push(struct transport *transport,
--
1.7.9
^ permalink raw reply related
* [PATCH 1/5] git rev-list: fix invalid typecast
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>
git rev-list passes rev_list_info, not rev_list objects. Without this
fix, rev-list enables or disables the --verify-objects option depending
on a read from an undefined memory location.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
builtin/rev-list.c | 4 ++--
t/t1450-fsck.sh | 26 ++++++++++++++++++++++++++
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ab3be7c..264e3ae 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -180,10 +180,10 @@ static void show_object(struct object *obj,
const struct name_path *path, const char *component,
void *cb_data)
{
- struct rev_info *info = cb_data;
+ struct rev_list_info *info = cb_data;
finish_object(obj, path, component, cb_data);
- if (info->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
+ if (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)
parse_object(obj->sha1);
show_object_with_name(stdout, obj, path, component);
}
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 523ce9c..5b8ebd8 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -191,4 +191,30 @@ test_expect_success 'cleaned up' '
test_cmp empty actual
'
+test_expect_success 'rev-list --verify-objects' '
+ git rev-list --verify-objects --all >/dev/null 2>out &&
+ test_cmp empty out
+'
+
+test_expect_success 'rev-list --verify-objects with bad sha1' '
+ sha=$(echo blob | git hash-object -w --stdin) &&
+ old=$(echo $sha | sed "s+^..+&/+") &&
+ new=$(dirname $old)/ffffffffffffffffffffffffffffffffffffff &&
+ sha="$(dirname $new)$(basename $new)" &&
+ mv .git/objects/$old .git/objects/$new &&
+ test_when_finished "remove_object $sha" &&
+ git update-index --add --cacheinfo 100644 $sha foo &&
+ test_when_finished "git read-tree -u --reset HEAD" &&
+ tree=$(git write-tree) &&
+ test_when_finished "remove_object $tree" &&
+ cmt=$(echo bogus | git commit-tree $tree) &&
+ test_when_finished "remove_object $cmt" &&
+ git update-ref refs/heads/bogus $cmt &&
+ test_when_finished "git update-ref -d refs/heads/bogus" &&
+
+ test_might_fail git rev-list --verify-objects refs/heads/bogus >/dev/null 2>out &&
+ cat out &&
+ grep -q "error: sha1 mismatch 63ffffffffffffffffffffffffffffffffffffff" out
+'
+
test_done
--
1.7.9
^ permalink raw reply related
* [PATCH 0/5] verify refs early
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhazobto3.fsf@alter.siamese.dyndns.org>
Hi Junio,
This series centers around "[PATCH 3/5] git push: verify refs early",
which implements your suggestions in reply to my "[PATCH] optionally
deny all pushes". I am not pursuing the deny config option further,
because as you noted, it is of course made redundant by a simple
pre-receive hook.
[PATCH 1/5] git rev-list: fix invalid typecast
I consider this one a fairly important bugfix. Although I was unable to
determine under which circumstances the bug takes effect, rev-list
probably takes a performance hit when it does.
[PATCH 2/5] do not override receive-pack errors
[PATCH 3/5] git push: verify refs early
Note that [PATCH 3/5] makes an extension to the transfer protocol.
[PATCH 4/5] t5541: use configured port number
A mostly unrelated bugfix.
[PATCH 5/5] push/fetch/clone --no-progress suppresses progress
A totally unrelated change. However, this conflicts textually
(lexically?) with [PATCH 3/5], and therefore I am including it here.
Clemens
^ permalink raw reply
* [PATCH 4/5] t5541: use configured port number
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
t/t5541-http-push.sh | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index d66ed24..cc6f081 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -106,7 +106,7 @@ cat >exp <<EOF
remote: error: hook declined to update refs/heads/dev2
To http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git
! [remote rejected] dev2 -> dev2 (hook declined)
-error: failed to push some refs to 'http://127.0.0.1:5541/smart/test_repo.git'
+error: failed to push some refs to 'http://127.0.0.1:$LIB_HTTPD_PORT/smart/test_repo.git'
EOF
test_expect_success 'rejected update prints status' '
--
1.7.9
^ permalink raw reply related
* [PATCH 3/5] git push: verify refs early
From: Clemens Buchacher @ 2012-02-13 20:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1329164235-29955-1-git-send-email-drizzd@aon.at>
Receive-pack waits for the transfer of all new object data to complete
before taking a closer look at the received refs. It may then decide to
refuse updates of refs without even looking at the data.
Instead, do as much ref validation as possible before new object data is
available, in order to avoid a potentially expensive transfer.
Receive-pack will then respond with "verify-refs ok" if at least one ref
update is valid. Otherwise, it will respond with "verify-refs no valid
refs".
On the client side, send-pack will wait for "verify-refs ok" or skip the
object transfer entirely if it reads a packet line that starts with
"verify-refs " but does not match "verify-refs ok". Any other response
is considered an error.
Early ref validation is disabled for the smart HTTP protocol, since it
tries to fit the entire push (ref info and object data) into a single
POST request.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
I suppose with some effort, this could be done for smart HTTP as well.
But I am not sure if we actually want the overhead of the additional
ping-pong for HTTP.
builtin/receive-pack.c | 83 ++++++++++++++++++++++++++++++++++++++----------
builtin/send-pack.c | 43 +++++++++++++++++--------
send-pack.h | 3 +-
3 files changed, 97 insertions(+), 32 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 0afb8b2..0129d9c 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -34,6 +34,8 @@ static int unpack_limit = 100;
static int report_status;
static int use_sideband;
static int quiet;
+static int verify_refs;
+static int stateless_rpc;
static int prefer_ofs_delta = 1;
static int auto_update_server_info;
static int auto_gc = 1;
@@ -123,7 +125,7 @@ static void show_ref(const char *path, const unsigned char *sha1)
else
packet_write(1, "%s %s%c%s%s\n",
sha1_to_hex(sha1), path, 0,
- " report-status delete-refs side-band-64k quiet",
+ " report-status delete-refs side-band-64k quiet verify-refs",
prefer_ofs_delta ? " ofs-delta" : "");
sent_capabilities = 1;
}
@@ -410,14 +412,13 @@ static void refuse_unconfigured_deny_delete_current(void)
rp_error("%s", refuse_unconfigured_deny_delete_current_msg[i]);
}
-static const char *update(struct command *cmd)
+static const char *verify_ref(struct command *cmd)
{
const char *name = cmd->ref_name;
struct strbuf namespaced_name_buf = STRBUF_INIT;
const char *namespaced_name;
unsigned char *old_sha1 = cmd->old_sha1;
unsigned char *new_sha1 = cmd->new_sha1;
- struct ref_lock *lock;
/* only refs/... are allowed */
if (prefixcmp(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -444,12 +445,6 @@ static const char *update(struct command *cmd)
}
}
- if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
- error("unpack should have generated %s, "
- "but I can't find it!", sha1_to_hex(new_sha1));
- return "bad pack";
- }
-
if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
if (deny_deletes && !prefixcmp(name, "refs/heads/")) {
rp_error("denying ref deletion for %s", name);
@@ -473,6 +468,27 @@ static const char *update(struct command *cmd)
}
}
+ return NULL;
+}
+
+static const char *update(struct command *cmd)
+{
+ const char *name = cmd->ref_name;
+ struct strbuf namespaced_name_buf = STRBUF_INIT;
+ const char *namespaced_name;
+ unsigned char *old_sha1 = cmd->old_sha1;
+ unsigned char *new_sha1 = cmd->new_sha1;
+ struct ref_lock *lock;
+
+ strbuf_addf(&namespaced_name_buf, "%s%s", get_git_namespace(), name);
+ namespaced_name = strbuf_detach(&namespaced_name_buf, NULL);
+
+ if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
+ error("unpack should have generated %s, "
+ "but I can't find it!", sha1_to_hex(new_sha1));
+ return "bad pack";
+ }
+
if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
!is_null_sha1(old_sha1) &&
!prefixcmp(name, "refs/heads/")) {
@@ -692,10 +708,41 @@ static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
return -1; /* end of list */
}
+static int verify_ref_commands(struct command *commands)
+{
+ unsigned char sha1[20];
+ int commands_ok;
+ struct command *cmd;
+
+ free(head_name_to_free);
+ head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
+
+ commands_ok = 0;
+ for (cmd = commands; cmd; cmd = cmd->next) {
+ cmd->error_string = verify_ref(cmd);
+ if (!cmd->error_string)
+ commands_ok++;
+ }
+
+ if (verify_refs && !stateless_rpc) {
+ struct strbuf buf = STRBUF_INIT;
+
+ packet_buf_write(&buf, "verify-refs %s\n",
+ commands_ok > 0 ? "ok" : "no valid refs");
+
+ if (use_sideband)
+ send_sideband(1, 1, buf.buf, buf.len, use_sideband);
+ else
+ safe_write(1, buf.buf, buf.len);
+ strbuf_release(&buf);
+ }
+
+ return commands_ok;
+}
+
static void execute_commands(struct command *commands, const char *unpacker_error)
{
struct command *cmd;
- unsigned char sha1[20];
if (unpacker_error) {
for (cmd = commands; cmd; cmd = cmd->next)
@@ -718,9 +765,6 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
check_aliased_updates(commands);
- free(head_name_to_free);
- head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
-
for (cmd = commands; cmd; cmd = cmd->next) {
if (cmd->error_string)
continue;
@@ -766,6 +810,8 @@ static struct command *read_head_info(void)
use_sideband = LARGE_PACKET_MAX;
if (parse_feature_request(feature_list, "quiet"))
quiet = 1;
+ if (parse_feature_request(feature_list, "verify-refs"))
+ verify_refs = 1;
}
cmd = xcalloc(1, sizeof(struct command) + len - 80);
hashcpy(cmd->old_sha1, old_sha1);
@@ -905,7 +951,6 @@ static int delete_only(struct command *commands)
int cmd_receive_pack(int argc, const char **argv, const char *prefix)
{
int advertise_refs = 0;
- int stateless_rpc = 0;
int i;
char *dir = NULL;
struct command *commands;
@@ -962,11 +1007,15 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
return 0;
if ((commands = read_head_info()) != NULL) {
+ int commands_ok;
const char *unpack_status = NULL;
- if (!delete_only(commands))
- unpack_status = unpack();
- execute_commands(commands, unpack_status);
+ commands_ok = verify_ref_commands(commands);
+ if (!verify_refs || commands_ok > 0) {
+ if (!delete_only(commands))
+ unpack_status = unpack();
+ execute_commands(commands, unpack_status);
+ }
if (pack_lockfile)
unlink_or_warn(pack_lockfile);
if (report_status)
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 71f258e..7d514ca 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -265,6 +265,8 @@ int send_pack(struct send_pack_args *args,
use_sideband = 1;
if (!server_supports("quiet"))
args->quiet = 0;
+ if (server_supports("verify-refs"))
+ args->verify_refs = 1;
if (!remote_refs) {
fprintf(stderr, "No refs in common and none specified; doing nothing.\n"
@@ -303,12 +305,13 @@ int send_pack(struct send_pack_args *args,
char *old_hex = sha1_to_hex(ref->old_sha1);
char *new_hex = sha1_to_hex(ref->new_sha1);
- if (!cmds_sent && (status_report || use_sideband || args->quiet)) {
- packet_buf_write(&req_buf, "%s %s %s%c%s%s%s",
+ if (!cmds_sent && (status_report || use_sideband || args->quiet || args->verify_refs)) {
+ packet_buf_write(&req_buf, "%s %s %s%c%s%s%s%s",
old_hex, new_hex, ref->name, 0,
status_report ? " report-status" : "",
use_sideband ? " side-band-64k" : "",
- args->quiet ? " quiet" : "");
+ args->quiet ? " quiet" : "",
+ args->verify_refs ? " verify-refs" : "");
}
else
packet_buf_write(&req_buf, "%s %s %s",
@@ -341,17 +344,29 @@ int send_pack(struct send_pack_args *args,
in = demux.out;
}
- if (new_refs && cmds_sent) {
- if (pack_objects(out, remote_refs, extra_have, args) < 0) {
- for (ref = remote_refs; ref; ref = ref->next)
- ref->status = REF_STATUS_NONE;
- if (args->stateless_rpc)
- close(out);
- if (git_connection_is_socket(conn))
- shutdown(fd[0], SHUT_WR);
- if (use_sideband)
- finish_async(&demux);
- return -1;
+ if (cmds_sent) {
+ int verify_refs_status = 0;
+
+ if (args->verify_refs && !args->stateless_rpc) {
+ char line[1000];
+ int len = packet_read_line(in, line, sizeof(line));
+ if (len < 15 || memcmp(line, "verify-refs ", 12))
+ return error("did not receive remote status");
+ verify_refs_status = memcmp(line, "verify-refs ok\n", 15);
+ }
+
+ if (!verify_refs_status && new_refs) {
+ if (pack_objects(out, remote_refs, extra_have, args) < 0) {
+ for (ref = remote_refs; ref; ref = ref->next)
+ ref->status = REF_STATUS_NONE;
+ if (args->stateless_rpc)
+ close(out);
+ if (git_connection_is_socket(conn))
+ shutdown(fd[0], SHUT_WR);
+ if (use_sideband)
+ finish_async(&demux);
+ return -1;
+ }
}
}
if (args->stateless_rpc && cmds_sent)
diff --git a/send-pack.h b/send-pack.h
index 05d7ab1..87edaa5 100644
--- a/send-pack.h
+++ b/send-pack.h
@@ -11,7 +11,8 @@ struct send_pack_args {
use_thin_pack:1,
use_ofs_delta:1,
dry_run:1,
- stateless_rpc:1;
+ stateless_rpc:1,
+ verify_refs:1;
};
int send_pack(struct send_pack_args *args,
--
1.7.9
^ permalink raw reply related
* Re: [RFC PATCH 0/3] git-p4: move to toplevel
From: Junio C Hamano @ 2012-02-13 20:00 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: git, Luke Diamand, Vitor Antunes
In-Reply-To: <1329070423-23761-1-git-send-email-pw@padd.com>
Pete Wyckoff <pw@padd.com> writes:
> Users install git-p4 currently by copying the git-p4 script from
> contrib/fast-import into a local or personal bin directory, and
> setting up an alias for "git p4" to invoke it.
Erm,... do you really need the alias if you add git-p4 in a directory on
your $PATH?
^ permalink raw reply
* Re: [PATCH v2 2/2] submodules: always use a relative path from gitdir to work tree
From: Junio C Hamano @ 2012-02-13 19:59 UTC (permalink / raw)
To: Jens Lehmann; +Cc: Git Mailing List, Antony Male, Phil Hord
In-Reply-To: <4F338156.9090507@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> The first version was whitespace damaged, please use this instead.
Thanks. This one looks somewhat iffy (1/2 looked perfectly fine, though).
> + a=$(cd "$gitdir" && pwd)
> + b=$(cd "$path" && pwd)
> + while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
> + do
> + a=${a#*/} b=${b#*/};
> + done
> + rel=$(echo $a | sed -e 's|[^/]*|..|g')
> + (clear_local_git_env; cd "$path" && git config core.worktree "$rel/$b")
> }
The style ([ "$b" ] vs "test -n "$b") aside, it strikes me odd that you
only check $b; it is unclear what guarantees that "$a" is always longer.
Maybe there is a reason but then a one-line comment here would not hurt?
^ permalink raw reply
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Jakub Narebski @ 2012-02-13 19:58 UTC (permalink / raw)
To: Michal Kiedrowicz; +Cc: git
In-Reply-To: <20120213075451.1bc20885@mkiedrowicz.ivo.pl>
Michal Kiedrowicz wrote:
> Jakub Narebski <jnareb@gmail.com> wrote:
> > Jakub Narebski <jnareb@gmail.com> writes:
[...]
> > > Anyway I have send to git mailing list a patch series, which in one
> > > of patches adds esc_html_match_hl($str, $regexp) to highlight
> > > matches in a string.
>
> Yeah, I saw that but after seeing that they accept different arguments
> I decided to leave them alone.
>
> > > Your esc_html_mark_range(), after a
> > > generalization, could be used as underlying "engine".
> > >
> > > Something like this, perhaps (untested):
>
> I think I'll leave it to you after merging both these series to
> master :)
Yes, you are right. Let's do it when we actually need it.
[...]
> > > # Highlight selected fragments of string, using given CSS class,
> > > # and escape HTML. It is assumed that fragments do not overlap.
> > > # Regions are passed as list of pairs (array references).
> > > sub esc_html_hl {
[...]
> > Actually we can accomodate both operating on string and operating on
> > array of characters in a single subroutine. Though it can be left for
> > later commit, anyway...
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [BUG] git-merge-octopus creates an empty merge commit with one parent
From: Junio C Hamano @ 2012-02-13 19:51 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <20120213202039.5346fa89@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> ... Except that I still think this behavior is not correct.
Well, what was missing from my "don't do it then" quote was "Doctor it
HURTS when I do this."; there is no question that it is not correct if it
hurts ;-).
Patches welcome; I suspect it shouldn't be too intrusive or difficult.
^ permalink raw reply
* Re: [BUG] git-merge-octopus creates an empty merge commit with one parent
From: Michał Kiedrowicz @ 2012-02-13 19:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vipjafu27.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> wrote:
> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
>
> > This happens when git merge is run to merge multiple commits that are
> > descendants of current HEAD (or are HEAD).
>
> I am reasonably sure you meant ancestors here.
I meant: ... to merge commits whose parent, grand-parent or
grand-grand-...-parent is HEAD. Commits to which HEAD may be
fast-forwarded.
>
> > to origin/master but accidentaly we called (while being on master):
> >
> > $ git merge master origin/master
>
> I am very tempted to throw this into "don't do it then" category.
I'm all for it. This is not a thing I would want to be doing. At
no time I would call this "a serious bug that needs fixing right
now". Except that I still think this behavior is not correct. git-merge
should not create commits that even aren't merges (they have single
parrent) and have exactly the same tree as its parent, and give it a
message "merge". Isn't this against the normal behavior of Git:
forwarding if possible and refusing to create commits that don't change
any file?
^ permalink raw reply
* Re: [PATCH] builtin/tag.c: Fix a sparse warning
From: Tom Grennan @ 2012-02-13 19:15 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <4F395569.8030106@ramsay1.demon.co.uk>
On Mon, Feb 13, 2012 at 06:24:41PM +0000, Ramsay Jones wrote:
>
>In particular, sparse complains as follows:
>
> SP builtin/tag.c
> builtin/tag.c:411:5: warning: symbol 'parse_opt_points_at' was \
> not declared. Should it be static?
>
>In order to suppress the warning, since the parse_opt_points_at()
>function does not need to be an external symbol, we simply add the
>static modifier to the function definition.
>
>Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
Acked-by: Tom Grennan <tmgrennan@gmail.com>
Thanks
>---
>
> builtin/tag.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
>diff --git a/builtin/tag.c b/builtin/tag.c
>index 8cfaaf8..fe7e5e5 100644
>--- a/builtin/tag.c
>+++ b/builtin/tag.c
>@@ -408,7 +408,7 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
> return check_refname_format(sb->buf, 0);
> }
>
>-int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
>+static int parse_opt_points_at(const struct option *opt __attribute__((unused)),
> const char *arg, int unset)
> {
> unsigned char sha1[20];
>--
>1.7.9
>
>
^ permalink raw reply
* Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
From: Jakub Narebski @ 2012-02-13 19:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: rajesh boyapati, git
In-Reply-To: <7vmx8mfu8k.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > Anyway, here is the patch that should fix those "CGI: fatal: Not a valid
> > object name HEAD" errors for you.
>
> I have to wonder if it is simpler and less error prone to check HEAD
> before doing anything else immediately after which repository is being
> consulted, and give the same "no history at all yet in this project" page
> for most if not all operations, instead of patching things up at this deep
> in the callchain.
Actually the problem is that there is history (there are other branches),
but HEAD points to unborn (unused) 'master' branch.
But you are right that we should fix underlying issue with invalid
assumption which gitweb uses, that HEAD points to a valid commit if
repository is non-empty.
Though I think leaving safety of 2nd patch could be a good idea anyway.
And 1st patch fixes a real issue, and does not only provide band-aid.
P.S. I will resend those three patches as patch series for easier review,
so they are not tangled in this deep thread. They should be in 'gitweb/web'
branch in my public forks of git repository on repo.or.cz and github.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: Setting up a Git server (+ gitweb) with .htaccess files HOWTO
From: Junio C Hamano @ 2012-02-13 19:10 UTC (permalink / raw)
To: Matthieu Moy; +Cc: git
In-Reply-To: <vpqbop266ak.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> I've set up a Git server on a machine on which I have a webspace,
> permission to run CGI scripts, but no shell or root access. Good news:
> it worked :-).
>
> I've documented the process here in case anyone's interested:
>
> http://www-verimag.imag.fr/~moy/?Host-a-Git-repository-over-HTTP-S
After seeing the section that runs "git init" in a throw-away CGI script,
I started wondering what the point of this site in forbidding a shell
access in the first place.
After all, if you changed that example with a script that feeds its input
to a shell, you would have enough "shell access" to do anything you need.
So...
Well, my reaction at this point was "why bother?", but then I realized
that that reaction is meant for the administrator of that hosting site,
not a user of that site, i.e. you.
Thanks for an amusing read, anyway.
^ permalink raw reply
* Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
From: Jakub Narebski @ 2012-02-13 19:04 UTC (permalink / raw)
To: rajesh boyapati; +Cc: git
In-Reply-To: <CA+EqV8xin_ubOoGouhHz2qnzoHrpMMQsjUTXnrtmsxRTLPZtZQ@mail.gmail.com>
On Mon, 13 Feb 2012, rajesh boyapati wrote:
> 2012/2/13 Jakub Narebski <jnareb@gmail.com>
>> On Mon, 13 Feb 2012, rajesh boyapati wrote:
>>> I am getting this error with this patch
>>>>>>>>>>>>>>>>>>>>>
>>> [2012-02-13 11:20:19,268] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: usage: git rev-list
>>> [OPTION] <commit-id>... [ -- paths... ]
>>> [2012-02-13 11:20:19,268] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: limiting output:
>>> [2012-02-13 11:20:19,268] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: --max-count=nr
>> [...]
>>> [2012-02-13 11:20:27,913] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
>>> used after filename
>>> [2012-02-13 11:20:32,579] ERROR
>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
>>> used after filename
>>> <<<<<<<<<<<<<<<<<<<
>>
>> Strange, I cannot reproduce this with non-Gerrit gitweb. It looks
>> like it somehow lost in between "... -- 2>/dev/null" at the end of
>> git-rev-list command, and fails at "--2" which is bad flag.
>>
> This is the patch I applied
>>>>>>>>>>>>>
> sub parse_commit {
> my ($commit_id) = @_;
> my %co;
>
> return unless defined $commit_id;
>
> local $/ = "\0";
>
> *open my $fd, "-|", quote_command(
> git_cmd(), "rev-list",*
> "--parents",
> "--header",
> "--max-count=1",
> $commit_id,
> * "--") . '2>/dev/null',*
^^^^^^^^^^^^^
It should be ' 2>/dev/null', with space before redirection, and not
'2>/dev/null'. This space is here necessary.
> With this Patch, Gerrit's gitweb is not showing anything.
> I mean, I can access gitweb from gerrit, but if I click on Tabs(like log,
> commit, etc...which worked with previous patches), I cannot see any thing.
>
> Even with previous patches also there is no improvement in Gerrit's gitweb,
> only some of the errors are gone in error_log.
> The improvement I am talking about is "If I click other tabs(log, shortlog,
> commit, tree,etc) after clicking "summary", Gerrit's gitweb is not showing
> anything".
Many views in gitweb do default to HEAD. If HEAD does not point to a valid
commit, they would fail, in better or worse way.
Except for the first one those patches are more of band-aid and workaround
than fixing underlying issue that gitweb assumes that HEAD is valid in
non-empty repository. But fixing this will require more work.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: git cherry doesn't list a merge commit
From: Junio C Hamano @ 2012-02-13 18:57 UTC (permalink / raw)
To: Tajti Ákos; +Cc: git
In-Reply-To: <4F392976.9070009@intland.com>
Tajti Ákos <akos.tajti@intland.com> writes:
> However, the manual of git cherry says:
> "*Every* commit that doesn’t exist in the <upstream> branch has its
> id (sha1) reported, prefixed by a symbol. The ones that have
> equivalent change already in the <upstream> branch are prefixed with a
> minus (-) sign"
The sole purpose of the command is to decide which commits in the existing
history to run "format-patch" on to feed the output to "am" to update the
other history, and it was clear for both readers and the author of the
documentation that a "commit" meant a "non-merge commit" in that context.
A patch to rephrase it to "every non-merge commit" for clarity may not be
useless, though.
^ permalink raw reply
* Re: Bulgarian translation of git
From: Junio C Hamano @ 2012-02-13 18:52 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jiang Xin, Git List, Ævar Arnfjörð Bjarmason,
Alexander Shopov (Александър Шопов)
In-Reply-To: <20120213133957.GA4838@burratino>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Hopefully that would make it easier for translators to keep both
> tracks well maintained at the same time.
I somehow think it is not such a big deal if l10n messages are not
maintained for maintenance tracks at all, actually, given that our cycles
for the master track have been 8-10 weeks long, which is not a long time
to wait, compared to 5 years people survived without any ;-).
^ permalink raw reply
* Re: [BUG] git-merge-octopus creates an empty merge commit with one parent
From: Junio C Hamano @ 2012-02-13 18:48 UTC (permalink / raw)
To: Michał Kiedrowicz; +Cc: git
In-Reply-To: <1329133736-20817-1-git-send-email-michal.kiedrowicz@gmail.com>
Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> This happens when git merge is run to merge multiple commits that are
> descendants of current HEAD (or are HEAD).
I am reasonably sure you meant ancestors here.
> to origin/master but accidentaly we called (while being on master):
>
> $ git merge master origin/master
I am very tempted to throw this into "don't do it then" category.
^ permalink raw reply
* Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
From: Junio C Hamano @ 2012-02-13 18:44 UTC (permalink / raw)
To: Jakub Narebski; +Cc: rajesh boyapati, git
In-Reply-To: <201202111402.31684.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> Anyway, here is the patch that should fix those "CGI: fatal: Not a valid
> object name HEAD" errors for you.
I have to wonder if it is simpler and less error prone to check HEAD
before doing anything else immediately after which repository is being
consulted, and give the same "no history at all yet in this project" page
for most if not all operations, instead of patching things up at this deep
in the callchain.
^ permalink raw reply
* Re: [PATCH 6/8] gitweb: Highlight interesting parts of diff
From: Jakub Narebski @ 2012-02-13 18:44 UTC (permalink / raw)
To: Michal Kiedrowicz; +Cc: git
In-Reply-To: <20120213074125.3e8793ad@mkiedrowicz.ivo.pl>
On Mon, 13 Feb 2012, Michal Kiedrowicz wrote:
> Jakub Narebski <jnareb@gmail.com> wrote:
>
> > Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> >
> > > Reading diff output is sometimes very hard, even if it's colored,
> > > especially if lines differ only in few characters. This is often
> > > true when a commit fixes a typo or renames some variables or
> > > functions.
> > >
> > This is certainly nice feature to have. I think most tools that
> > implement side-by-side diff implement this too.
> >
> > > This commit teaches gitweb to highlight characters that are
> > > different between old and new line. This should work in the
> > > similar manner as in Trac or GitHub.
> > >
> > Doe you have URLs with good examples of such diff refinement
> > highlighting (GNU Emacs ediff/emerge terminology) / highlighting
> > differing segments (diff-highlight terminology)?
>
> I haven't found *examples* on GitHub and Trac sites, but what about
> these ones:
>
> https://github.com/gitster/git/commit/8cad4744ee37ebec1d9491a1381ec1771a1ba795
> http://trac.edgewall.org/changeset/10973
Thanks. That is what I meant by "good examples". Perhaps they should
be put in the commit message?
BTW GitHub is closed source, but we can check what algorithm does Trac
use for diff refinement highlighting (highlighting changed portions of
diff).
> > > The code that compares lines is based on
> > > contrib/diff-highlight/diff-highlight, except that it works with
> > > multiline changes too. It also won't highlight lines that are
> > > completely different because that would only make the output
> > > unreadable.
> > >
> > So what does it look like? From the contrib/diff-highlight/README I
> > guess that it finds common prefix and common suffix, and highlights
> > the rest, which includes change.
>
> Yes.
>
> > It doesn't implement LCS / diff
> > algorithm like e.g. tkdiff does for its diff refinement highlighting,
> > isn't it?
>
> I doesn't. I share the Jeff's opinion that:
> a) Jeff's approach is "good enough"
> b) LCS on bytes could be very confusing if it marked few sets of
> characters.
I wonder if we can use --diff-words for diff refinement highlighting,
i.e. LCS on words.
Anyway Jeff's approach is a bit limited, in that it would work only for
change that does not involve adding newlines, for example splitting
overly long line when changing something.
See for example line 1786 (in pre-image) in http://trac.edgewall.org/changeset/10973
> > By completly different you mean that they do not have common prefix or
> > common suffix (at least one of them), isn't it?
BTW. is it "at least one of prefix or suffix are non-empty" or "both prefix
and suffix are non-empty"?
> Yes, but I also don't highlight lines which prefix/suffix consists only
> of whitespace (This is quite common).
O.K., that is quite sensible.
> I would also consider ignoring prefixes/suffixes with punctuation, like:
>
> - * I like you.
> + * Alice had a little lamb.
But this patch doesn't implement this feature yet, isn't it?
Well, here is another idea: do not highlight if sum of prefix and suffix
lengths are less than some threshold, e.g. 2 characters not including
whitespace, or some percentage with respect to total line length.
> > > +# Highlight characters from $prefix to $suffix and escape HTML.
> > > +# $str is a reference to the array of characters.
> > > +sub esc_html_mark_range {
> > > + my ($str, $prefix, $suffix) = @_;
> > > +
> > > + # Don't generate empty <span> element.
> > > + if ($prefix == $suffix + 1) {
> > > + return esc_html(join('', @$str), -nbsp=>1);
> > > + }
> > > +
> > > + my $before = join('', @{$str}[0..($prefix - 1)]);
> > > + my $marked = join('', @{$str}[$prefix..$suffix]);
> > > + my $after = join('', @{$str}[($suffix + 1)..$#{$str}]);
It would be nicer with 'part' from List::MoreUtils... but that module
is unfortunately not in core.
> > Eeeeeek! First you split into letters, in caller at that, then join?
> > Why not pass striung ($str suggests string not array of characters),
> > and use substr instead?
> >
> > [Please disregard this and the next paragraph at first reading]
>
> I will rename $str to something more self describing.
Please do.
BTW. don't you assume here that both common prefix and common suffix
are non-empty?
[...]
> > > +
> > > +# Format removed and added line, mark changed part and HTML-format
> > > them.
> >
> > You should probably add here that this code is taken from
> > diff-highlight in contrib area, isn't it?
>
> True.
And perhaps not the changes from diff-highlight, unless you meant to
send them upstream for inclusion.
> > > +sub format_rem_add_line {
> > > + my ($rem, $add) = @_;
> > > + my @r = split(//, $rem);
> > > + my @a = split(//, $add);
BTW the name of variable can be just @add and @rem.
> > > + my ($esc_rem, $esc_add);
> > > + my ($prefix, $suffix_rem, $suffix_add) = (1, $#r, $#a);
> >
> > It is not as much $prefix, as $prefix_len, isn't it?
>
> Yes.
>
> > Shouldn't
> > $prefix / $prefix_len start from 0, not from 1?
>
> It starts from 1 because it skips first +/-. It should become obvious
> after reading the comment from last patch :).
>
> + # In combined diff we must ignore two +/- characters.
> + $prefix = 2 if ($is_combined);
Anyway comment about that fact would be nice.
> > > + my ($prefix_is_space, $suffix_is_space) = (1, 1);
> > > +
> > > + while ($prefix < @r && $prefix < @a) {
> > > + last if ($r[$prefix] ne $a[$prefix]);
> > > +
> > > + $prefix_is_space = 0 if ($r[$prefix] !~ /\s/);
> > > + $prefix++;
> > > + }
> >
> > Ah, I see that it is easier to find common prefix by treating string
> > as array of characters.
> >
> > Though I wonder if it wouldn't be easier to use trick of XOR-ing two
> > strings (see "Bitwise String Operators" in perlop(1)):
> >
> > my $xor = "$rem" ^ "$add";
> >
> > and finding starting sequence of "\0", which denote common prefix.
> >
> >
> > Though this and the following is a nice implementation of
> > algorithm... as it would be implemented in C. Nevermind, it might be
> > good enough...
>
> The splitting and comparing by characters is taken from diff-highlight.
> I don't think it's worth changing here.
You are right.
I'll try to come with hacky algorithm using string bitwise xor and regexp,
and benchmark it comparing to your C-like solution, but it can be left for
later (simple is better than clever, usually).
> > > + while ($suffix_rem >= $prefix && $suffix_add >= $prefix) {
> > > + last if ($r[$suffix_rem] ne $a[$suffix_add]);
> > > +
> > > + $suffix_is_space = 0 if ($r[$suffix_rem] !~ /\s/);
> > > + $suffix_rem--;
> > > + $suffix_add--;
> > > + }
> >
> > BTW., perhaps using single negative $suffix_len instead of separate
> > $suffix_rem_pos and $suffix_add_pos would make code simpler and easier
> > to understand?
>
> I'll try that.
I think this should just work, and it would be one variable less to
track.
> > > # HTML-format diff context, removed and added lines.
> > > sub format_ctx_rem_add_lines {
> > > - my ($ctx, $rem, $add) = @_;
> > > + my ($ctx, $rem, $add, $is_combined) = @_;
> > > my (@new_ctx, @new_rem, @new_add);
> > > + my $num_add_lines = @$add;
> >
> > Why is this temporary variable needed? If you are not sure that ==
> > operator enforces scalar context on both arguments you can always
> > write
> >
> > scalar @$add == scalar @$rem
>
> You read my mind.
BTW. the same comment applies to patch adding support for highlighting
changed part in combined diff.
> > The original contrib/diff-highlight works only for single changed line
> > (single removed and single added). You make this code work also for
> > the case where number of aded lines is equal to the number of removed
> > lines, and assume that subsequent changed lines in preimage
> > correcponds to subsequent changed lines in postimage, [...]
[...]
> > This is not described in commit message, I think.
--
Jakub Narebski
Poland
^ permalink raw reply
* [PATCH] builtin/tag.c: Fix a sparse warning
From: Ramsay Jones @ 2012-02-13 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: tmgrennan, GIT Mailing-list
In particular, sparse complains as follows:
SP builtin/tag.c
builtin/tag.c:411:5: warning: symbol 'parse_opt_points_at' was \
not declared. Should it be static?
In order to suppress the warning, since the parse_opt_points_at()
function does not need to be an external symbol, we simply add the
static modifier to the function definition.
Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
builtin/tag.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 8cfaaf8..fe7e5e5 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -408,7 +408,7 @@ static int strbuf_check_tag_ref(struct strbuf *sb, const char *name)
return check_refname_format(sb->buf, 0);
}
-int parse_opt_points_at(const struct option *opt __attribute__ ((unused)),
+static int parse_opt_points_at(const struct option *opt __attribute__((unused)),
const char *arg, int unset)
{
unsigned char sha1[20];
--
1.7.9
^ permalink raw reply related
* Re: User authentication in GIT
From: supadhyay @ 2012-02-13 18:19 UTC (permalink / raw)
To: git
In-Reply-To: <CAMK1S_g5mk44vhTsFG4-kzMw7jMVvX0VK6pXAY=PFAgOEsccgw@mail.gmail.com>
Thanks for suggesting the link, but would like to know which method is the
most secure and optimal method to use.
For testing purpose we migrate our repositories from CVS2GIT but now having
issue wiht user managment. How to manage it ? We have users for different
repositories and having different access, somewhere I read about using
Gitolite I can mange users but not find the efficent and useful method to
use it.
Thanks .
..
--
View this message in context: http://git.661346.n2.nabble.com/User-authentication-in-GIT-tp7261349p7281349.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
From: Jakub Narebski @ 2012-02-13 18:15 UTC (permalink / raw)
To: rajesh boyapati; +Cc: git
In-Reply-To: <CA+EqV8xTsavQFWsoijrt+0UcfxSZO2voL=CawrRPvDeB=qHQfg@mail.gmail.com>
On Mon, 13 Feb 2012, rajesh boyapati wrote:
> 2012/2/11 Jakub Narebski <jnareb@gmail.com>
>> On Thu, 9 Feb 2012, Jakub Narebski wrote:
>>> On Wed, 8 Feb 2012, rajesh boyapati wrote:
>>>> 2012/2/8 Jakub Narebski <jnareb@gmail.com>
[...]
>>>> When I applied the above patch and also the patch from your previous
>>>> e-mail, I am getting this error
>>>>>>>>>>>>>>>>>
>>>> [2012-02-08 14:09:58,396] ERROR
>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
>>>> 'HEAD'
>>>> [2012-02-08 14:10:06,732] ERROR
>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
>>>> 'HEAD'
>>>> [2012-02-08 14:10:11,404] ERROR
>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad revision
>>>> 'HEAD'
>>>> [2012-02-08 14:10:15,270] ERROR
>>>> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: Not a valid
>>>> object name HEAD
>>>> <<<<<<<<<<<<<<
>>>> With these patches, the previous errors at line numbers are gone.
[...]
>>> This final issue will be a bit harder to fix. This error message
>>>
>>> fatal: bad revision 'HEAD'
>>>
>>> comes from git (I think from "git rev-list" command), and not from gitweb.
>>> It is printed on STDERR of git command. What has to be done to fix it is
>>> to capture stderr of a process, or silence it.
[...]
>> Anyway, here is the patch that should fix those "CGI: fatal: Not a valid
>> object name HEAD" errors for you.
>>
>> I'll resend the all the patches as single patch series for inclusion in
>> git, but I am not sure if this latest patch will be accepted because of
>> drawbacks of its implementation.
>>
>> -->8 ---- ----- ----- ----->8 --
>> From: Jakub Narebski <jnareb@gmail.com>
>> Subject: [PATCH] gitweb: Silence stderr in parse_commit*() subroutines
[...]
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 1181aeb..081ac45 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -3338,12 +3338,13 @@ sub parse_commit {
>>
>> local $/ = "\0";
>>
>> - open my $fd, "-|", git_cmd(), "rev-list",
>> + open my $fd, "-|", quote_command(
>> + git_cmd(), "rev-list",
>> "--parents",
>> "--header",
>> "--max-count=1",
>> $commit_id,
>> - "--",
>> + "--") . ' 2>/dev/null',
>> or die_error(500, "Open git-rev-list failed");
>> my $commit_text = <$fd>;
>> %co = parse_commit_text($commit_text, 1)
>> @@ -3363,7 +3364,8 @@ sub parse_commits {
>>
>> local $/ = "\0";
>>
>> - open my $fd, "-|", git_cmd(), "rev-list",
>> + open my $fd, "-|", quote_command(
>> + git_cmd(), "rev-list",
>> "--header",
>> @args,
>> ("--max-count=" . $maxcount),
>> @@ -3371,7 +3373,7 @@ sub parse_commits {
>> @extra_options,
>> $commit_id,
>> "--",
>> - ($filename ? ($filename) : ())
>> + ($filename ? ($filename) : ())) . ' 2>/dev/null'
>> or die_error(500, "Open git-rev-list failed");
>> while (my $line = <$fd>) {
>> my %co = parse_commit_text($line);
>>
>
> I am getting this error with this patch
>>>>>>>>>>>>>>>>>>>
> [2012-02-13 11:20:19,268] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: usage: git rev-list
> [OPTION] <commit-id>... [ -- paths... ]
> [2012-02-13 11:20:19,268] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: limiting output:
> [2012-02-13 11:20:19,268] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: --max-count=nr
[...]
> [2012-02-13 11:20:27,913] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
> used after filename
> [2012-02-13 11:20:32,579] ERROR
> com.google.gerrit.httpd.gitweb.GitWebServlet : CGI: fatal: bad flag '--2'
> used after filename
> <<<<<<<<<<<<<<<<<<<
Strange, I cannot reproduce this with non-Gerrit gitweb. It looks
like it somehow lost in between "... -- 2>/dev/null" at the end of
git-rev-list command, and fails at "--2" which is bad flag.
Are you sure you applied the patch correctly? Does 'object' view
(take any 'commit' or 'blob' or 'tree' view, and replace action part
by 'object') works correctly in Gerrit's gitweb?
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [PATCHv2 1/4] refs: add common refname_match_patterns()
From: Tom Grennan @ 2012-02-13 17:27 UTC (permalink / raw)
To: Michael Haggerty; +Cc: pclouds, git, gitster, jasampler
In-Reply-To: <4F3898F8.2000201@alum.mit.edu>
On Mon, Feb 13, 2012 at 06:00:40AM +0100, Michael Haggerty wrote:
>On 02/11/2012 08:17 PM, Tom Grennan wrote:
>> Yes, I didn't explicitly state that the precedence is the order written
>> and in correctly described the first case. How about?
>>
>> /**
>> * Returns in highest to lowest precedence:
>> * 1 with an empty patterns list
>> * 0 if refname fnmatch()es any ^ prefaced pattern
>> * 1 if refname fnmatch()es any other pattern
>> * 0 otherwise
>> */
>
>Much better; thanks.
>
>Please note that this choice of semantics limits its power. For
>example, if the rule were instead (like with gitattributes(5)) "if more
>than one pattern matches a refname, a later pattern overrides an earlier
>pattern", then one could do things like
>
> refs/remotes/*/* !refs/remotes/gitster/* refs/remotes/gitster/master
>
>to include specific references within a hierarchy that is otherwise
>excluded.
>
>However, since rev-list apparently uses a rule more like the one that
>you are proposing, it might be better to be consistent than to choose a
>different convention.
Hmm, I think it's important to have same respective result in each of
these case's,
$ git tag -l | grep v1.7.8.*
$ git tag -l v1.7.8*
$ git tag -l | grep -v .*-rc*
$ git tag -l ^*-rc*
$ git tag -l v1.7.8* | grep -v .*-rc*
$ git tag -l v1.7.8* ^*-rc*
$ git tag -l ^*-rc* v1.7.8*
What I propose is somewhat analogous to gitignore's double negative,
* An optional prefix ! which negates the pattern; any matching
file excluded by a previous pattern will become included again. If
a negated pattern matches, this will override lower precedence
patterns sources.
I still prefer "^" to "!" b/c A) it doesn't cause the noted regressions;
and B) doesn't need command quoting. I'd accept the counter proposals
of --exclude or --with[out][-TYPE] but frankly, that's more
code/documentation churn ("less code is always better"[TM]) and worse,
more crap to type on the command line:
$ git --with-tags v1.7.8* --without-tags '*-rc*' tag -l v1.7.8*
or
$ git tag -l --exclude '*-rc*' v1.7.8*
vs.
$ git tag -l v1.7.8* ^*-rc*
--
TomG
^ permalink raw reply
* [PATCH] completion: --no-abbrev-commit for git-log and git-show
From: Michael Schubert @ 2012-02-13 16:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
Signed-off-by: Michael Schubert <mschub@elegosoft.com>
---
contrib/completion/git-completion.bash | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index d7367e9..22d7018 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1587,7 +1587,7 @@ _git_log ()
$__git_log_gitk_options
--root --topo-order --date-order --reverse
--follow --full-diff
- --abbrev-commit --abbrev=
+ --abbrev-commit --no-abbrev-commit --abbrev=
--relative-date --date=
--pretty= --format= --oneline
--cherry-pick
@@ -2384,7 +2384,7 @@ _git_show ()
return
;;
--*)
- __gitcomp "--pretty= --format= --abbrev-commit --oneline
+ __gitcomp "--pretty= --format= --abbrev-commit --no-abbrev-commit --online
$__git_diff_common_options
"
return
--
1.7.9.324.g3d1db
^ permalink raw reply related
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