* [PATCH v5 2/2] gitweb: append short hash ids to snapshot files
From: Mark Rada @ 2009-09-26 17:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, git
Teach gitweb how to produce nicer snapshot names by only using the
short hash id. If clients make requests using a tree-ish that is not a
partial or full SHA-1 hash, then the short hash will also be appended
to whatever they asked for.
This also includes tests cases for t9502-gitweb-standalone-parse-output.
Signed-off-by: Mark Rada <marada@uwaterloo.ca>
---
Changes since v4:
- moved git_get_full_hash into this commit
- changed test case format, suggested by Junio
- explicity request at least a length of 7 for short hashes
gitweb/gitweb.perl | 40 +++++++++++++++--
t/t9502-gitweb-standalone-parse-output.sh | 67 +++++++++++++++++++++++++++++
2 files changed, 103 insertions(+), 4 deletions(-)
create mode 100644 t/t9502-gitweb-standalone-parse-output.sh
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8d4a2ae..bc132a5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1983,14 +1983,39 @@ sub quote_command {
# get HEAD ref of given project as hash
sub git_get_head_hash {
+ return git_get_full_hash(shift, 'HEAD');
+}
+
+sub git_get_full_hash {
my $project = shift;
+ my $hash = shift;
my $o_git_dir = $git_dir;
my $retval = undef;
$git_dir = "$projectroot/$project";
- if (open my $fd, "-|", git_cmd(), "rev-parse", "--verify", "HEAD") {
- my $head = <$fd>;
+ if (open my $fd, '-|', git_cmd(), 'rev-parse', '--verify', $hash) {
+ $hash = <$fd>;
close $fd;
- if (defined $head && $head =~ /^([0-9a-fA-F]{40})$/) {
+ if (defined $hash && $hash =~ /^([0-9a-fA-F]{40})$/) {
+ $retval = $1;
+ }
+ }
+ if (defined $o_git_dir) {
+ $git_dir = $o_git_dir;
+ }
+ return $retval;
+}
+
+# try and get a shorter hash id
+sub git_get_short_hash {
+ my $project = shift;
+ my $hash = shift;
+ my $o_git_dir = $git_dir;
+ my $retval = undef;
+ $git_dir = "$projectroot/$project";
+ if (open my $fd, '-|', git_cmd(), 'rev-parse', '--short=7', $hash) {
+ $hash = <$fd>;
+ close $fd;
+ if (defined $hash && $hash =~ /^([0-9a-fA-F]{7,})$/) {
$retval = $1;
}
}
@@ -5203,6 +5228,13 @@ sub git_snapshot {
die_error(400, 'Object is not a tree-ish');
}
+
+ my $full_hash = git_get_full_hash($project, $hash);
+ if ($full_hash =~ /^$hash/) {
+ $hash = git_get_short_hash($project, $hash);
+ } else {
+ $hash .= '-' . git_get_short_hash($project, $hash);
+ }
my $name = $project;
$name =~ s,([^/])/*\.git$,$1,;
$name = basename($name);
@@ -5213,7 +5245,7 @@ sub git_snapshot {
$cmd = quote_command(
git_cmd(), 'archive',
"--format=$known_snapshot_formats{$format}{'format'}",
- "--prefix=$name/", $hash);
+ "--prefix=$name/", $full_hash);
if (exists $known_snapshot_formats{$format}{'compressor'}) {
$cmd .= ' | ' . quote_command(@{$known_snapshot_formats{$format}{'compressor'}});
}
diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
new file mode 100644
index 0000000..5f2b1d5
--- /dev/null
+++ b/t/t9502-gitweb-standalone-parse-output.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+#
+# Copyright (c) 2009 Mark Rada
+#
+
+test_description='gitweb as standalone script (parsing script output).
+
+This test runs gitweb (git web interface) as a CGI script from the
+commandline, and checks that it produces the correct output, either
+in the HTTP header or the actual script output.'
+
+
+. ./gitweb-lib.sh
+
+# ----------------------------------------------------------------------
+# snapshot file name
+
+test_commit \
+ 'SnapshotFileTests' \
+ 'i can has snapshot?'
+
+test_expect_success 'snapshots: give full hash' '
+ ID=`git rev-parse --verify HEAD` &&
+ gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+ ID=`git rev-parse --short HEAD` &&
+ grep ".git-$ID.tar.gz" gitweb.output
+'
+test_debug 'cat gitweb.output'
+
+test_expect_success 'snapshots: give short hash' '
+ ID=`git rev-parse --short HEAD` &&
+ gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+ grep ".git-$ID.tar.gz" gitweb.output
+'
+test_debug 'cat gitweb.output'
+
+test_expect_success 'snapshots: give almost full hash' '
+ ID=`git rev-parse --short=30 HEAD` &&
+ gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+ ID=`git rev-parse --short HEAD` &&
+ grep ".git-$ID.tar.gz" gitweb.output
+'
+test_debug 'cat gitweb.output'
+
+test_expect_success 'snapshots: give HEAD tree-ish' '
+ gitweb_run "p=.git;a=snapshot;h=HEAD;sf=tgz" &&
+ ID=`git rev-parse --short HEAD` &&
+ grep ".git-HEAD-$ID.tar.gz" gitweb.output
+'
+test_debug 'cat gitweb.output'
+
+test_expect_success 'snapshots: give branch name tree-ish' '
+ gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+ ID=`git rev-parse --short master` &&
+ grep ".git-master-$ID.tar.gz" gitweb.output
+'
+test_debug 'cat gitweb.output'
+
+test_expect_success 'snapshots: give tag tree-ish' '
+ gitweb_run "p=.git;a=snapshot;h=SnapshotFileTests;sf=tgz" &&
+ ID=`git rev-parse --short SnapshotFileTests` &&
+ grep ".git-SnapshotFileTests-$ID.tar.gz" gitweb.output
+'
+test_debug 'cat gitweb.output'
+
+
+test_done
--
1.6.4.GIT
^ permalink raw reply related
* [PATCH] instaweb: support mod_cgid for apache2
From: Mark Rada @ 2009-09-26 18:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
Some people have mod_cgid instead of mod_cgi, most likely as a result of
choosing a threaded MPM.
In cases where the user has both modules, mod_cgi will be preferred in
order to maintain a simpler setup.
This patch also causes instaweb to print a message and die in cases
where there is no module that instaweb knows how to handle.
Signed-off-by: Mark Rada <marada@uwaterloo.ca>
---
I thought I would squeeze this patch in while I was at it today.
git-instaweb.sh | 16 +++++++++++++++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/git-instaweb.sh b/git-instaweb.sh
index d96eddb..622a5f0 100755
--- a/git-instaweb.sh
+++ b/git-instaweb.sh
@@ -317,7 +317,21 @@ EOF
resolve_full_httpd
list_mods=$(echo "$full_httpd" | sed "s/-f$/-l/")
$list_mods | grep 'mod_cgi\.c' >/dev/null 2>&1 || \
- echo "LoadModule cgi_module $module_path/mod_cgi.so" >> "$conf"
+ if test -f "$module_path/mod_cgi.so"
+ then
+ echo "LoadModule cgi_module $module_path/mod_cgi.so" >> "$conf"
+ else
+ $list_mods | grep 'mod_cgid\.c' >/dev/null 2>&1 || \
+ if test -f "$module_path/mod_cgid.so"
+ then
+ echo "LoadModule cgid_module $module_path/mod_cgid.so" \
+ >> "$conf"
+ else
+ echo "You have no CGI support!"
+ exit 2
+ fi
+ echo "ScriptSock logs/gitweb.sock" >> "$conf"
+ fi
cat >> "$conf" <<EOF
AddHandler cgi-script .cgi
<Location /gitweb.cgi>
--
1.6.4.GIT
^ permalink raw reply related
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Giuseppe Scrivano @ 2009-09-26 18:21 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0909261756510.4985@pacific.mpi-cbg.de>
Hello,
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I understand that clang complains when argc is not really used afterwards,
> but do we really want to do this? I mean, if somebody decides it'd be a
> good idea to check the number of arguments after parsing the arguments,
> they might be bitten by the fact that it is now actively wrong.
probably this is not the only case to leave as it is. I just cleaned
anything clang reported.
Cheers,
Giuseppe
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Sverre Rabbelier @ 2009-09-26 18:34 UTC (permalink / raw)
To: Giuseppe Scrivano; +Cc: Johannes Schindelin, git
In-Reply-To: <871vltefdj.fsf@master.homenet>
Heya,
On Sat, Sep 26, 2009 at 20:21, Giuseppe Scrivano <gscrivano@gnu.org> wrote:
> probably this is not the only case to leave as it is. I just cleaned
> anything clang reported.
Then it would probably have been better to say so by at least marking
your patch as RFC and including such a remark in the cover letter, no?
Also, now that this has been pointed out, you shouldn't expect it to
be included until someone either takes your patch and cleans it up (as
in, checks all statements manually), or until you do so yourself.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Giuseppe Scrivano @ 2009-09-26 18:46 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Johannes Schindelin, git
In-Reply-To: <fabb9a1e0909261134qd90dba1n9637fe4adc253fc1@mail.gmail.com>
Hello,
Sverre Rabbelier <srabbelier@gmail.com> writes:
> Then it would probably have been better to say so by at least marking
> your patch as RFC and including such a remark in the cover letter, no?
> Also, now that this has been pointed out, you shouldn't expect it to
> be included until someone either takes your patch and cleans it up (as
> in, checks all statements manually), or until you do so yourself.
I really had to include a RFC remark. After what Johannes reported, I
think there is need only to restore assignments to argc while other ones
can be dropped without problems. I'll post a cleaned patch later.
Cheers,
Giuseppe
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Giuseppe Scrivano @ 2009-09-26 19:15 UTC (permalink / raw)
To: git; +Cc: Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <fabb9a1e0909261134qd90dba1n9637fe4adc253fc1@mail.gmail.com>
Here is a cleaned patch. I think these assignments can be removed
without any problem.
Cheers,
Giuseppe
>From 7501d82998132b15ad5cda78c0650f4f4a0b0e93 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano@gnu.org>
Date: Sat, 26 Sep 2009 21:11:21 +0200
Subject: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
---
builtin-commit.c | 2 +-
builtin-fetch--tool.c | 2 +-
builtin-fetch-pack.c | 2 +-
builtin-grep.c | 2 --
builtin-pack-objects.c | 2 +-
builtin-receive-pack.c | 8 ++++----
builtin-send-pack.c | 2 +-
builtin-show-branch.c | 4 ++--
color.c | 2 +-
compat/mkstemps.c | 2 +-
connect.c | 1 -
diff.c | 2 +-
http-fetch.c | 3 +--
transport.c | 4 ++--
upload-pack.c | 2 +-
15 files changed, 18 insertions(+), 22 deletions(-)
diff --git a/builtin-commit.c b/builtin-commit.c
index 200ffda..331d2a0 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -1035,7 +1035,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
parents = reduce_heads(parents);
} else {
reflog_msg = "commit";
- pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
+ commit_list_insert(lookup_commit(head_sha1), pptr);
}
/* Finally, get the commit message */
diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 3dbdf7a..c47469f 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp,
note_len += sprintf(note + note_len, "%s ", kind);
note_len += sprintf(note + note_len, "'%s' of ", what);
}
- note_len += sprintf(note + note_len, "%.*s", remote_len, remote);
+ sprintf(note + note_len, "%.*s", remote_len, remote);
fprintf(fp, "%s\t%s\t%s\n",
sha1_to_hex(commit ? commit->object.sha1 : sha1),
not_for_merge ? "not-for-merge" : "",
diff --git a/builtin-fetch-pack.c b/builtin-fetch-pack.c
index 629735f..583f4e3 100644
--- a/builtin-fetch-pack.c
+++ b/builtin-fetch-pack.c
@@ -555,7 +555,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
}
if (*hdr_arg)
*av++ = hdr_arg;
- *av++ = NULL;
+ *av = NULL;
cmd.in = demux.out;
cmd.git_cmd = 1;
diff --git a/builtin-grep.c b/builtin-grep.c
index 761799d..d36b59e 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -400,7 +400,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)
if (sizeof(randarg) <= len)
die("maximum length of args exceeded");
push_arg(argptr);
- argptr += len;
}
}
else {
@@ -410,7 +409,6 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)
if (sizeof(randarg) <= len)
die("maximum length of args exceeded");
push_arg(argptr);
- argptr += len;
}
}
for (p = opt->pattern_list; p; p = p->next) {
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 02f9246..bea7141 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -2307,7 +2307,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
*/
if (!pack_to_stdout)
- base_name = argv[i++];
+ base_name = argv[i];
if (pack_to_stdout != !base_name)
usage(pack_usage);
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index b771fe9..82d1564 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -368,7 +368,7 @@ static char update_post_hook[] = "hooks/post-update";
static void run_update_post_hook(struct command *cmd)
{
struct command *cmd_p;
- int argc, status;
+ int argc;
const char **argv;
for (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
@@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd)
argc++;
}
argv[argc] = NULL;
- status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
+ run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
| RUN_COMMAND_STDOUT_TO_STDERR);
}
@@ -506,7 +506,7 @@ static const char *unpack(void)
if (receive_fsck_objects)
unpacker[i++] = "--strict";
unpacker[i++] = hdr_arg;
- unpacker[i++] = NULL;
+ unpacker[i] = NULL;
code = run_command_v_opt(unpacker, RUN_GIT_CMD);
if (!code)
return NULL;
@@ -528,7 +528,7 @@ static const char *unpack(void)
keeper[i++] = "--fix-thin";
keeper[i++] = hdr_arg;
keeper[i++] = keep_arg;
- keeper[i++] = NULL;
+ keeper[i] = NULL;
memset(&ip, 0, sizeof(ip));
ip.argv = keeper;
ip.out = -1;
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 37e528e..5afd542 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
if (args->use_ofs_delta)
argv[i++] = "--delta-base-offset";
if (args->quiet)
- argv[i++] = "-q";
+ argv[i] = "-q";
memset(&po, 0, sizeof(po));
po.argv = argv;
po.in = -1;
diff --git a/builtin-show-branch.c b/builtin-show-branch.c
index 3510a86..e567eb5 100644
--- a/builtin-show-branch.c
+++ b/builtin-show-branch.c
@@ -191,9 +191,9 @@ static void name_commits(struct commit_list *list,
break;
}
if (nth == 1)
- en += sprintf(en, "^");
+ sprintf(en, "^");
else
- en += sprintf(en, "^%d", nth);
+ sprintf(en, "^%d", nth);
name_commit(p, xstrdup(newname), 0);
i++;
name_first_parent_chain(p);
diff --git a/color.c b/color.c
index 62977f4..5b31588 100644
--- a/color.c
+++ b/color.c
@@ -110,7 +110,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,
}
}
if (bg >= 0) {
- if (sep++)
+ if (sep)
*dst++ = ';';
if (bg < 8) {
*dst++ = '4';
diff --git a/compat/mkstemps.c b/compat/mkstemps.c
index 14179c8..dbf916e 100644
--- a/compat/mkstemps.c
+++ b/compat/mkstemps.c
@@ -45,7 +45,7 @@ int gitmkstemps(char *pattern, int suffix_len)
template[2] = letters[v % num_letters]; v /= num_letters;
template[3] = letters[v % num_letters]; v /= num_letters;
template[4] = letters[v % num_letters]; v /= num_letters;
- template[5] = letters[v % num_letters]; v /= num_letters;
+ template[5] = letters[v % num_letters];
fd = open(pattern, O_CREAT | O_EXCL | O_RDWR, 0600);
if (fd > 0)
diff --git a/connect.c b/connect.c
index 7945e38..da6c7c1 100644
--- a/connect.c
+++ b/connect.c
@@ -18,7 +18,6 @@ static int check_ref(const char *name, int len, unsigned int flags)
/* Skip the "refs/" part */
name += 5;
- len -= 5;
/* REF_NORMAL means that we don't want the magic fake tag refs */
if ((flags & REF_NORMAL) && check_ref_format(name) < 0)
diff --git a/diff.c b/diff.c
index e1be189..e75f58e 100644
--- a/diff.c
+++ b/diff.c
@@ -901,7 +901,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
/* Find the longest filename and max number of changes */
reset = diff_get_color_opt(options, DIFF_RESET);
- set = diff_get_color_opt(options, DIFF_PLAIN);
+ diff_get_color_opt(options, DIFF_PLAIN);
add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
diff --git a/http-fetch.c b/http-fetch.c
index e8f44ba..6879904 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -3,7 +3,6 @@
int main(int argc, const char **argv)
{
- const char *prefix;
struct walker *walker;
int commits_on_stdin = 0;
int commits;
@@ -19,7 +18,7 @@ int main(int argc, const char **argv)
int get_verbosely = 0;
int get_recover = 0;
- prefix = setup_git_directory();
+ setup_git_directory();
git_config(git_default_config, NULL);
diff --git a/transport.c b/transport.c
index 644a30a..8ec0df6 100644
--- a/transport.c
+++ b/transport.c
@@ -308,7 +308,7 @@ static int rsync_transport_push(struct transport *transport,
args[i++] = "info";
args[i++] = get_object_directory();
args[i++] = buf.buf;
- args[i++] = NULL;
+ args[i] = NULL;
if (run_command(&rsync))
return error("Could not push objects to %s",
@@ -334,7 +334,7 @@ static int rsync_transport_push(struct transport *transport,
args[i++] = "--ignore-existing";
args[i++] = temp_dir.buf;
args[i++] = rsync_url(transport->url);
- args[i++] = NULL;
+ args[i] = NULL;
if (run_command(&rsync))
result = error("Could not push to %s",
rsync_url(transport->url));
diff --git a/upload-pack.c b/upload-pack.c
index 38ddac2..fb3436c 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -241,7 +241,7 @@ static void create_pack_file(void)
argv[arg++] = "--delta-base-offset";
if (use_include_tag)
argv[arg++] = "--include-tag";
- argv[arg++] = NULL;
+ argv[arg] = NULL;
memset(&pack_objects, 0, sizeof(pack_objects));
pack_objects.in = shallow_nr ? rev_list.out : -1;
--
1.6.3.3
^ permalink raw reply related
* Re: how optparse can go horribly wrong
From: Shawn O. Pearce @ 2009-09-26 19:25 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Nicolas Sebrecht, git
In-Reply-To: <fabb9a1e0909260644w781d3c3h4fca22e3b7d97768@mail.gmail.com>
Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Sat, Sep 26, 2009 at 03:51, Nicolas Sebrecht <nicolas.s.dev@gmx.fr> wrote:
> > Doing this only to -m flag would break consistency. That said, I don't
> > have any opinion in disallowing the sticked form for _all_ short
> > options.
>
> Perhaps instead disallow it for short options that do not take a
> one-symbol argument, that is -n4 makes a lot of sense to me, but -m"my
> commit message here" not so much.
-1 on that, because long, long, long ago when I worked on -m support
for commit I remember insisting that -mfoo and -m foo should be
the same. I often do `git commit -a -mwip` or something to save my
branch state and come back later.
What I think we should do is not allow cuddling of short options
when the final option takes more than 1 character worth of argument.
Thus `git commit -a -mfoo` is OK, but `git commit -amfoo` is not.
--
Shawn.
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: René Scharfe @ 2009-09-26 19:28 UTC (permalink / raw)
To: Giuseppe Scrivano; +Cc: git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <87ske9cya9.fsf@master.homenet>
> diff --git a/diff.c b/diff.c
> index e1be189..e75f58e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -901,7 +901,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>
> /* Find the longest filename and max number of changes */
> reset = diff_get_color_opt(options, DIFF_RESET);
> - set = diff_get_color_opt(options, DIFF_PLAIN);
> + diff_get_color_opt(options, DIFF_PLAIN);
> add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
> del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
diff_get_color_opt() has no side-effects; the changed line is a no-op.
René
^ permalink raw reply
* Re: [JGIT PATCH 1/9] mavenizing step 1: moved over the initial poms from Jasons branch Signed-off-by: Mark Struberg <struberg@yahoo.de>
From: Mark Struberg @ 2009-09-26 19:50 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, spearce, Jason van Zyl
In-Reply-To: <200909252333.56756.robin.rosenberg.lists@dewire.com>
Hi Robin!
a) Actually git-format-patch only created 0001-0009 so there is no 0/9.
b) 2/9 is the actual directory structure moving. I received it, but since it is pretty large (330k already with -M -l0) it might got filtered out?
If so then may I ask you to please fetch it from http://github.com/sonatype/JGit branch 'mavenize'? It has the same content I sent to the list.
txs and LieGrue,
strub
--- On Fri, 9/25/09, Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> From: Robin Rosenberg <robin.rosenberg.lists@dewire.com>
> Subject: Re: [JGIT PATCH 1/9] mavenizing step 1: moved over the initial poms from Jasons branch Signed-off-by: Mark Struberg <struberg@yahoo.de>
> To: "Mark Struberg" <struberg@yahoo.de>
> Cc: git@vger.kernel.org, spearce@spearce.org, "Jason van Zyl" <jvanzyl@sonatype.com>
> Date: Friday, September 25, 2009, 11:33 PM
>
> Where are the 0/9 and 2/9 mails?
>
> -- robin
>
>
>
^ permalink raw reply
* Re: [PATCH] make 'git clone' ask the remote only for objects it cares about
From: Shawn O. Pearce @ 2009-09-26 19:50 UTC (permalink / raw)
To: Nicolas Pitre
Cc: Junio C Hamano, Jason Merrill, Matthieu Moy, git, Hin-Tak Leung
In-Reply-To: <alpine.LFD.2.00.0909252314260.4997@xanadu.home>
Nicolas Pitre <nico@fluxnic.net> wrote:
> Current behavior of 'git clone' when not using --mirror is to fetch
> everything from the peer, and then filter out unwanted refs just before
> writing them out to the cloned repository. This may become highly
> inefficient if the peer has an unusual ref namespace, or if it simply
> has "remotes" refs of its own, and those locally unwanted refs are
> connecting to a large set of objects which becomes unreferenced as soon
> as they are fetched.
...
> +static void write_remote_refs(const struct ref *local_refs, const char *reflog)
Here reflog is now unused. I'm going to squash this in.
diff --git a/builtin-clone.c b/builtin-clone.c
index edf7c7f..4992c25 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -342,7 +342,7 @@ static struct ref *wanted_peer_refs(const struct ref *refs,
return local_refs;
}
-static void write_remote_refs(const struct ref *local_refs, const char *reflog)
+static void write_remote_refs(const struct ref *local_refs)
{
const struct ref *r;
@@ -534,7 +534,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
if (refs) {
clear_extra_refs();
- write_remote_refs(mapped_refs, reflog_msg.buf);
+ write_remote_refs(mapped_refs);
remote_head = find_ref_by_name(refs, "HEAD");
remote_head_points_at =
--
Shawn.
^ permalink raw reply related
* Re: [PATCH] git branch -D: give a better error message when lockfile creation fails
From: Shawn O. Pearce @ 2009-09-26 19:58 UTC (permalink / raw)
To: Miklos Vajna; +Cc: Jeff King, git
In-Reply-To: <1253972051-31980-1-git-send-email-vmiklos@frugalware.org>
Miklos Vajna <vmiklos@frugalware.org> wrote:
> On Fri, Sep 25, 2009 at 11:31:43PM -0400, Jeff King <peff@peff.net> wrote:
> > afterwards). So probably you would need to first refactor
> > unable_to_lock_index_die() to handle just printing the error without
> > dying.
>
> I removed the NORETURN macro as otherwise gcc would issue a warning, as
> it does not realise that unable_to_lock_index_die() never returns.
Please don't. If you refactor the error message formatting into
a static function called from the two extern ones, you can still
centralize the formatting but also keep NORETURN on the method that
doesn't return. The annotation is useful and should not be removed.
> -extern NORETURN void unable_to_lock_index_die(const char *path, int err);
> +extern int unable_to_lock_index(const char *path, int err, int noreturn);
> +extern void unable_to_lock_index_die(const char *path, int err);
--
Shawn.
^ permalink raw reply
* Re: [PATCH] git branch -D: give a better error message when lockfile creation fails
From: Matthieu Moy @ 2009-09-26 20:04 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Miklos Vajna, Jeff King, git
In-Reply-To: <20090926195812.GH14660@spearce.org>
"Shawn O. Pearce" <spearce@spearce.org> writes:
> Miklos Vajna <vmiklos@frugalware.org> wrote:
>> On Fri, Sep 25, 2009 at 11:31:43PM -0400, Jeff King <peff@peff.net> wrote:
>> > afterwards). So probably you would need to first refactor
>> > unable_to_lock_index_die() to handle just printing the error without
>> > dying.
>>
>> I removed the NORETURN macro as otherwise gcc would issue a warning, as
>> it does not realise that unable_to_lock_index_die() never returns.
>
> Please don't. If you refactor the error message formatting into
> a static function called from the two extern ones, you can still
> centralize the formatting but also keep NORETURN on the method that
> doesn't return. The annotation is useful and should not be removed.
I guess the lint-trap would be to add a
die("unable_to_lock_index() should have died already");
at the end of unable_to_lock_index_die().
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH 1/2] Make generated MSVC solution file open from Windows Explorer
From: Shawn O. Pearce @ 2009-09-26 20:05 UTC (permalink / raw)
To: Sebastian Schuberth; +Cc: git, mstormo
In-Reply-To: <bdca99240909260245i6ba10dd4j1b2ee9e74ea5282d@mail.gmail.com>
Sebastian Schuberth <sschuberth@gmail.com> wrote:
> On Sat, Sep 26, 2009 at 02:05, Shawn O. Pearce <spearce@spearce.org> wrote:
> > Odd. ??If I copy and paste from Thunderbird, its fine. ??But if I
> > save the body out as an attachment from mutt, it fails.
> >
> > I wonder if it has to do with the From header appearing in the top
> > of the body; this header has to be escaped with a leading space in
> > mbox format. ??It looks like Thunderbird might be doing some magic to
> > remove that leading space from the context lines, while mutt isn't.
> >
> > Next time, don't include the first From line?
>
> Will try. So what about these two patches? Should I re-send them with
> the first "From" stripped?
Might be worth trying. I honestly don't know why they were munged
before.
> Or will *.patch files that are attached to emails, instead of sending
> the patch inline, be accepted?
We really don't like them, because you can't comment on them inline
easily. Sometimes they are acceptable for translation files when
the character encoding otherwise gets really broken.
--
Shawn.
^ permalink raw reply
* Re: [JGIT PATCH 7/9] removing eclipse project files
From: Mark Struberg @ 2009-09-26 20:10 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: MatthiasSohn, git@vger.kernel.org, spearce@spearce.org
In-Reply-To: <200909252317.02296.robin.rosenberg.lists@dewire.com>
Hi Robin!
Thanks for your comments, answers inside.
Please note that all my comments are for JGit only and not for the Egit Eclipse plugin! This should be discussed separately.
LieGrue,
strub
--- On Fri, 9/25/09, Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> From: Robin Rosenberg <robin.rosenberg.lists@dewire.com>
> Subject: Re: [JGIT PATCH 7/9] removing eclipse project files
> To: "Mark Struberg" <struberg@yahoo.de>
> Cc: "MatthiasSohn" <matthias.sohn@sap.com>, "git@vger.kernel.org" <git@vger.kernel.org>, "spearce@spearce.org" <spearce@spearce.org>
> Date: Friday, September 25, 2009, 11:17 PM
> torsdag 24 september 2009 13:50:11
> skrev Mark Struberg <struberg@yahoo.de>:
> > Hi Matthias!
> >
> > the answer is a clear yes and no - a 'jein' for
> german speaking people like you ;)
> >
> > yes: we have the same settings for the compiler as
> used before: jdk 1.5 for source and target. This is exactly
> what has been taken in the ant build which was used prior to
> maven.
> >
> > Please note that the settings in
> org.eclipse.jdt.core.prefs never had (and must not have) any
> impact on the created jar!
>
> Not sure what ant files you are referring to here and which
> jars. The plugins downloadable from jgit.org has been built
> using PDE build. so some of the .settings should affect the
> compiler and thus the generated jars.
Apologise, you are right, no ant files. But a shell script make_jgit.sh which calls the javac directly. So no PDE for JGit as far as I see from the sources.
>
> > and no: currently the very file you mentioned contains
> a lot more stuff. In fact most of this are only editor
> settings, preferred formattings etc which has nothing to do
> with the build per se. Eclipse has the ability to
> import/export all those settings in a XML file which is
> version independent. We should go this way and also supply
> similar setting-files for Idea and NetBeans. But forcing
> those settings via an internal Eclipse plugin config file is
> imho bad practice.
>
> That way is awkward and people to import the settings and
> screw them up in their workspaces. I've made the projects
> I'm involved use .settings and make different settings
> mostly a non-issue because everyone gets new
> settings automatically as they change.
We should integrate the checkstyle plugin into our pom. This should give you almost the same functionality.
> Prior to eclipse 3.3
> sharing settings was a big problem, but It's not a big deal
> nowadays. The most annoying thing is that some settings are
> not available as project specific settings.
As you already pointed to: we have to clearly separate between settings stored in the project itself and settings stored in the workspace. The first are by far not all settings needed, the 2nd are not checked in to git anyway. Maybe I didn't find it yet, but is there an ability to set formatter settings for XML (e.g. Tabs vs spaces policy)? I was only able to specify this for the whole workspace and not on a per project basis. And there is a lot more which imho cannot be set for a project. So checking in the xml sounds like it is way more powerful isn't? And we would have this feature for a lot non-Eclipse users too (e.g. for Jonas who hacks the nbgit NetBeans plugin based on JGit (again: EGit is a different story!)).
>
> We use 3.3 (well I think the last user dropped it
> recently), 3.4 and 3.5. I often have to fix up new projects
> but that is typically a one-time per eclipse project
> problem. (typically the JRE gets bound to a specific install
> location).
>
> The .launch files is another story since they change format
> all the time.
And the profiler settings, and and and. It's sad, but the list is long :(
We can also let the eclipse settings files checked in currently if you like. But I'd be happy if we continue collecting information and then make a decision.
txs,
strub
^ permalink raw reply
* Re: [PATCH] git branch -D: give a better error message when lockfile creation fails
From: Matthieu Moy @ 2009-09-26 20:12 UTC (permalink / raw)
To: Miklos Vajna; +Cc: Jeff King, git
In-Reply-To: <1253972051-31980-1-git-send-email-vmiklos@frugalware.org>
Miklos Vajna <vmiklos@frugalware.org> writes:
> diff --git a/refs.c b/refs.c
> index 24865cf..4eb4fc7 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -972,8 +972,10 @@ static int repack_without_ref(const char *refname)
> if (!found)
> return 0;
> fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
> - if (fd < 0)
> + if (fd < 0) {
> + unable_to_lock_index(git_path("packed-refs"), errno, 0);
> return error("cannot delete '%s' from packed refs", refname);
> + }
If unable_to_lock_index is called for something other that the index,
it should probably be renamed.
Other than that, I like what the patch does (I'm the one who wrote the
message for the index ;-) ).
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Johannes Schindelin @ 2009-09-26 20:39 UTC (permalink / raw)
To: Giuseppe Scrivano; +Cc: git, Sverre Rabbelier
In-Reply-To: <87ske9cya9.fsf@master.homenet>
Hi,
On Sat, 26 Sep 2009, Giuseppe Scrivano wrote:
> diff --git a/builtin-commit.c b/builtin-commit.c
> index 200ffda..331d2a0 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -1035,7 +1035,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> parents = reduce_heads(parents);
> } else {
> reflog_msg = "commit";
> - pptr = &commit_list_insert(lookup_commit(head_sha1), pptr)->next;
> + commit_list_insert(lookup_commit(head_sha1), pptr);
> }
Sorry, but from the context it seems as if the same remark I had for argc
applies here, too. There are exactly three other similar-looking
assignments and it is too easy IMO to mess up when one want to rearrange
things there.
In other words, I deem the removal of this assignment worse than what we
have now -- at least in terms of how easy it is to modify the code safely.
I just looked further 3 hunks and had exactly the same impression there,
so I stopped looking.
Sorry,
Dscho
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Jeff King @ 2009-09-26 20:46 UTC (permalink / raw)
To: Giuseppe Scrivano; +Cc: git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <87ske9cya9.fsf@master.homenet>
On Sat, Sep 26, 2009 at 09:15:58PM +0200, Giuseppe Scrivano wrote:
> Here is a cleaned patch. I think these assignments can be removed
> without any problem.
I don't agree. For example:
> --- a/builtin-fetch--tool.c
> +++ b/builtin-fetch--tool.c
> @@ -169,7 +169,7 @@ static int append_fetch_head(FILE *fp,
> note_len += sprintf(note + note_len, "%s ", kind);
> note_len += sprintf(note + note_len, "'%s' of ", what);
> }
> - note_len += sprintf(note + note_len, "%.*s", remote_len, remote);
> + sprintf(note + note_len, "%.*s", remote_len, remote);
This is a very particular C idiom: you are building a string over
several statements using a function that adds to the string and tells
you how much it added. The implicit invariant of the note_len variable
is that it _always_ contains the current length, so each statement uses
it as input and pushes it forward on output.
Any experienced C programmer should look at that and be able to see
exactly what's going on. And people adding more lines don't need to
munge the existing lines; the invariant property of note_len means they
just need to add more, similar lines.
But your patch destroys that invariant. It makes it harder to see what's
going on, because it breaks the idiom. And it makes it more likely for
somebody adding a line further on to make a mistake (and certainly it
makes their patch harder to read and review, as they have to munge
unrelated lines).
So no, while there is no code _now_ that is relying on the invariant
being kept after the last statement (which is what the static analyzer
is finding out), the point is not for the compiler to realize that, but
for human programmers to see it.
So I think your version is less readable and maintainable. And it
doesn't even introduce any efficiency; any decent compiler should be
able to optimize out the addition.
> --- a/builtin-fetch-pack.c
> +++ b/builtin-fetch-pack.c
> @@ -555,7 +555,7 @@ static int get_pack(int xd[2], char **pack_lockfile)
> }
> if (*hdr_arg)
> *av++ = hdr_arg;
> - *av++ = NULL;
> + *av = NULL;
I would argue a similar same idiom exists here, though given that NULL
by definition is ending the av list it is somewhat less strong (i.e.,
there is already something to that statement that indicates that it
_must_ be the last one).
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -2307,7 +2307,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
> */
>
> if (!pack_to_stdout)
> - base_name = argv[i++];
> + base_name = argv[i];
And again here. Maintaining the invariant on 'i' is important to
readability and maintainability.
> --- a/builtin-receive-pack.c
> +++ b/builtin-receive-pack.c
> @@ -368,7 +368,7 @@ static char update_post_hook[] = "hooks/post-update";
> static void run_update_post_hook(struct command *cmd)
> {
> struct command *cmd_p;
> - int argc, status;
> + int argc;
> const char **argv;
>
> for (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
> @@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd)
> argc++;
> }
> argv[argc] = NULL;
> - status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
> + run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
> | RUN_COMMAND_STDOUT_TO_STDERR);
> }
Now this is one that I do think is sensible. The variable isn't used, so
don't even bother declaring it.
> @@ -506,7 +506,7 @@ static const char *unpack(void)
> if (receive_fsck_objects)
> unpacker[i++] = "--strict";
> unpacker[i++] = hdr_arg;
> - unpacker[i++] = NULL;
> + unpacker[i] = NULL;
> code = run_command_v_opt(unpacker, RUN_GIT_CMD);
> if (!code)
> return NULL;
Another invariant on 'i', though it has the NULL argument as above.
> @@ -528,7 +528,7 @@ static const char *unpack(void)
> keeper[i++] = "--fix-thin";
> keeper[i++] = hdr_arg;
> keeper[i++] = keep_arg;
> - keeper[i++] = NULL;
> + keeper[i] = NULL;
> memset(&ip, 0, sizeof(ip));
> ip.argv = keeper;
> ip.out = -1;
Ditto.
> diff --git a/builtin-send-pack.c b/builtin-send-pack.c
> index 37e528e..5afd542 100644
> --- a/builtin-send-pack.c
> +++ b/builtin-send-pack.c
> @@ -55,7 +55,7 @@ static int pack_objects(int fd, struct ref *refs, struct extra_have_objects *ext
> if (args->use_ofs_delta)
> argv[i++] = "--delta-base-offset";
> if (args->quiet)
> - argv[i++] = "-q";
> + argv[i] = "-q";
> memset(&po, 0, sizeof(po));
> po.argv = argv;
> po.in = -1;
Invariant on 'i'.
> diff --git a/builtin-show-branch.c b/builtin-show-branch.c
> index 3510a86..e567eb5 100644
> --- a/builtin-show-branch.c
> +++ b/builtin-show-branch.c
> @@ -191,9 +191,9 @@ static void name_commits(struct commit_list *list,
> break;
> }
> if (nth == 1)
> - en += sprintf(en, "^");
> + sprintf(en, "^");
> else
> - en += sprintf(en, "^%d", nth);
> + sprintf(en, "^%d", nth);
Building up string, invariant on 'en'.
And there are more examples of each. I'm not going to bother labeling
them all. But I really think any time you're removing an increment that
is meant to keep an invariant for future code, we should leave it as-is.
-Peff
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Reece Dunn @ 2009-09-26 21:03 UTC (permalink / raw)
To: Jeff King; +Cc: Giuseppe Scrivano, git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <20090926204604.GA2960@coredump.intra.peff.net>
2009/9/26 Jeff King <peff@peff.net>:
> On Sat, Sep 26, 2009 at 09:15:58PM +0200, Giuseppe Scrivano wrote:
>
>> Here is a cleaned patch. I think these assignments can be removed
>> without any problem.
>
>> --- a/builtin-receive-pack.c
>> +++ b/builtin-receive-pack.c
>> @@ -368,7 +368,7 @@ static char update_post_hook[] = "hooks/post-update";
>> static void run_update_post_hook(struct command *cmd)
>> {
>> struct command *cmd_p;
>> - int argc, status;
>> + int argc;
>> const char **argv;
>>
>> for (argc = 0, cmd_p = cmd; cmd_p; cmd_p = cmd_p->next) {
>> @@ -391,7 +391,7 @@ static void run_update_post_hook(struct command *cmd)
>> argc++;
>> }
>> argv[argc] = NULL;
>> - status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
>> + run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
>> | RUN_COMMAND_STDOUT_TO_STDERR);
>> }
>
> Now this is one that I do think is sensible. The variable isn't used, so
> don't even bother declaring it.
The status variable is removed in this patch.
But then shouldn't the status returned be checked and acted on? That
is, are failures from run_command_v_opt being reported to the user, or
otherwise reacted to?
In this case, IIUC, the status should be returned by the
run_update_post_hook function. I.e.:
- static void run_update_post_hook(struct command *cmd)
+ static int run_update_post_hook(struct command *cmd)
{
struct command *cmd_p;
- int argc, status;
+ int argc;
...
- status = run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
+ return run_command_v_opt(argv, RUN_COMMAND_NO_STDIN
| RUN_COMMAND_STDOUT_TO_STDERR);
}
Thus having the same effect (removing the status variable). Callers of
run_update_post_hook should be checked as well, as should other
run_command_* calls.
- Reece
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Jeff King @ 2009-09-26 21:12 UTC (permalink / raw)
To: Reece Dunn; +Cc: Giuseppe Scrivano, git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <3f4fd2640909261403n78a7e45cm3d2cd48408b5ff52@mail.gmail.com>
On Sat, Sep 26, 2009 at 10:03:27PM +0100, Reece Dunn wrote:
> > Now this is one that I do think is sensible. The variable isn't used, so
> > don't even bother declaring it.
>
> The status variable is removed in this patch.
Yes. Sorry if I wasn't clear, but what I meant was "this does not fall
under the same idioms as the other ones, and it is a fine thing to be
removing".
> But then shouldn't the status returned be checked and acted on? That
> is, are failures from run_command_v_opt being reported to the user, or
> otherwise reacted to?
Perhaps. This is the post-update hook, so at that point we have already
committed any changes to the repository. Usually it is used for running
"git update-server-info" for repositories available over dumb protocols.
So there is no useful action for receive-pack to do after seeing an
error. But I said "perhaps" above, because it might be useful to notify
the user over the stderr sideband that the hook failed. Even though we
have no action to take, the user might care or want to investigate a
potential problem.
I suspect nobody has cared about this before, though, because the stderr
channel for the hook is also directed to the user. So if
update-server-info (or whatever) fails, presumably it is complaining to
stderr and the user sees that. Adding an additional "by the way, your
hook failed" is just going to be noise in most cases.
> Thus having the same effect (removing the status variable). Callers of
> run_update_post_hook should be checked as well, as should other
> run_command_* calls.
There is exactly one caller, and it doesn't care about the return code
for the reasons mentioned above.
-Peff
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Reece Dunn @ 2009-09-26 21:20 UTC (permalink / raw)
To: Jeff King; +Cc: Giuseppe Scrivano, git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <20090926211220.GA3387@coredump.intra.peff.net>
2009/9/26 Jeff King <peff@peff.net>:
> On Sat, Sep 26, 2009 at 10:03:27PM +0100, Reece Dunn wrote:
>
>> > Now this is one that I do think is sensible. The variable isn't used, so
>> > don't even bother declaring it.
>>
>> The status variable is removed in this patch.
>
> Yes. Sorry if I wasn't clear, but what I meant was "this does not fall
> under the same idioms as the other ones, and it is a fine thing to be
> removing".
Sure.
>> But then shouldn't the status returned be checked and acted on? That
>> is, are failures from run_command_v_opt being reported to the user, or
>> otherwise reacted to?
>
> Perhaps. This is the post-update hook, so at that point we have already
> committed any changes to the repository. Usually it is used for running
> "git update-server-info" for repositories available over dumb protocols.
>
> So there is no useful action for receive-pack to do after seeing an
> error. But I said "perhaps" above, because it might be useful to notify
> the user over the stderr sideband that the hook failed. Even though we
> have no action to take, the user might care or want to investigate a
> potential problem.
>
> I suspect nobody has cared about this before, though, because the stderr
> channel for the hook is also directed to the user. So if
> update-server-info (or whatever) fails, presumably it is complaining to
> stderr and the user sees that. Adding an additional "by the way, your
> hook failed" is just going to be noise in most cases.
It could be used to return an error status from main if it is used in
a chained command in a script. Other than that, I agree.
>> Thus having the same effect (removing the status variable). Callers of
>> run_update_post_hook should be checked as well, as should other
>> run_command_* calls.
>
> There is exactly one caller, and it doesn't care about the return code
> for the reasons mentioned above.
Including being called from a script?
- Reece
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Jeff King @ 2009-09-26 21:36 UTC (permalink / raw)
To: Reece Dunn; +Cc: Giuseppe Scrivano, git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <3f4fd2640909261420h2588df4cld8dd3e49f9654e9e@mail.gmail.com>
On Sat, Sep 26, 2009 at 10:20:18PM +0100, Reece Dunn wrote:
> > I suspect nobody has cared about this before, though, because the stderr
> > channel for the hook is also directed to the user. So if
> > update-server-info (or whatever) fails, presumably it is complaining to
> > stderr and the user sees that. Adding an additional "by the way, your
> > hook failed" is just going to be noise in most cases.
>
> It could be used to return an error status from main if it is used in
> a chained command in a script. Other than that, I agree.
I'm not sure that's a good idea. Your push _did_ happen, and the remote
repo was updated. So you have no way of knowing from an error exit code
that changes were in fact made, and it was simply the post-update hook
failing.
Of course, you can argue that the current behavior is similarly broken:
on success, you have no idea if the post-update hook failed or not. But
I would argue that whether the push itself happened is more important
than whether the hook succeeded or not. If you really care, you should
either:
1. Use some sort of side channel to report hook status.
2. Use the pre-receive hook, which can abort the push if it wants to.
But all of that is "if we were designing this hook from scratch". At
this point, it doesn't make sense to change the semantics. People may be
relying on the current behavior, and in fact it is documented (in
githooks(5)):
This hook is meant primarily for notification, and cannot
affect the outcome of git-receive-pack.
> > There is exactly one caller, and it doesn't care about the return code
> > for the reasons mentioned above.
>
> Including being called from a script?
I suppose people can be scripting around "git receive-pack" itself,
though I find it pretty unlikely. I find it much more likely for them to
script around "git push", which calls receive-pack on the remote end,
and may or may not get the actual status (without checking, I imagine
the exit code is lost anyway for git:// pushes, but probably passed back
along for pushes over ssh).
At any rate, even if we assume that people are scripting around it, and
that they can in fact see the exit status, I think we would want to keep
it the same for compatibility reasons, as mentioned above.
-Peff
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Giuseppe Scrivano @ 2009-09-26 21:42 UTC (permalink / raw)
To: Reece Dunn; +Cc: Jeff King, git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <3f4fd2640909261420h2588df4cld8dd3e49f9654e9e@mail.gmail.com>
Reece Dunn <msclrhd@googlemail.com> writes:
>> There is exactly one caller, and it doesn't care about the return code
>> for the reasons mentioned above.
>
> Including being called from a script?
I agree, if something goes wrong then it should be reported. The same
applies to the `run_receive_hook' return code that is not checked in
`cmd_receive_pack'.
Considering you want to keep the current source code invariants, and I
don't have any objection to it, probably the only assignment that can be
removed is the following one:
>From f8dd14bf4c3f3e132f6a8e13bf3e2fc575a804b1 Mon Sep 17 00:00:00 2001
From: Giuseppe Scrivano <gscrivano@gnu.org>
Date: Sat, 26 Sep 2009 23:23:13 +0200
Subject: [PATCH] Remove a dead assignment found by the clang static analyzer
---
http-fetch.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/http-fetch.c b/http-fetch.c
index e8f44ba..6879904 100644
--- a/http-fetch.c
+++ b/http-fetch.c
@@ -3,7 +3,6 @@
int main(int argc, const char **argv)
{
- const char *prefix;
struct walker *walker;
int commits_on_stdin = 0;
int commits;
@@ -19,7 +18,7 @@ int main(int argc, const char **argv)
int get_verbosely = 0;
int get_recover = 0;
- prefix = setup_git_directory();
+ setup_git_directory();
git_config(git_default_config, NULL);
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Reece Dunn @ 2009-09-26 21:46 UTC (permalink / raw)
To: Jeff King; +Cc: Giuseppe Scrivano, git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <20090926213602.GA3756@coredump.intra.peff.net>
2009/9/26 Jeff King <peff@peff.net>:
> On Sat, Sep 26, 2009 at 10:20:18PM +0100, Reece Dunn wrote:
>
>> > I suspect nobody has cared about this before, though, because the stderr
>> > channel for the hook is also directed to the user. So if
>> > update-server-info (or whatever) fails, presumably it is complaining to
>> > stderr and the user sees that. Adding an additional "by the way, your
>> > hook failed" is just going to be noise in most cases.
>>
>> It could be used to return an error status from main if it is used in
>> a chained command in a script. Other than that, I agree.
>
> I'm not sure that's a good idea. Your push _did_ happen, and the remote
> repo was updated. So you have no way of knowing from an error exit code
> that changes were in fact made, and it was simply the post-update hook
> failing.
Ok.
> Of course, you can argue that the current behavior is similarly broken:
> on success, you have no idea if the post-update hook failed or not. But
> I would argue that whether the push itself happened is more important
> than whether the hook succeeded or not. If you really care, you should
> either:
>
> 1. Use some sort of side channel to report hook status.
>
> 2. Use the pre-receive hook, which can abort the push if it wants to.
>
> But all of that is "if we were designing this hook from scratch". At
> this point, it doesn't make sense to change the semantics. People may be
> relying on the current behavior, and in fact it is documented (in
> githooks(5)):
>
> This hook is meant primarily for notification, and cannot
> affect the outcome of git-receive-pack.
That's fine. As long as the behaviour is documented (which as you
pointed out, it is).
- Reece
^ permalink raw reply
* Re: git log --pretty=format:%h prints (unrequired) abbreviated sha
From: Marco Costalba @ 2009-09-26 21:55 UTC (permalink / raw)
To: alexandrul cuza; +Cc: git
In-Reply-To: <23e749600909261128v4e398dadhc8be096f2500b17e@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 467 bytes --]
Hi Alexandrul,
Now it works !!!! :-) :-)
It was a virus ! I have run Malwarebytes Anti-malware 1.41 and it
found some virus (I send you the logs in attachment), after removing
them the problem disappeared.
I have Kaspersky as antivirus, but, although a very good antivirus it
failed to identify them.
I didn't know this Malwarebytes, but it seems very powerful.
Thanks anyhow for your exceptional help, I have really appreciated that !!!!
Thanks again
Marco
[-- Attachment #2: mbam-log-2009-09-26 (22-26-18).txt --]
[-- Type: text/plain, Size: 2207 bytes --]
Malwarebytes' Anti-Malware 1.41
Database version: 2863
Windows 6.0.6002 Service Pack 2
26/09/2009 22.26.18
mbam-log-2009-09-26 (22-26-18).txt
Scan type: Full Scan (C:\|D:\|)
Objects scanned: 341703
Time elapsed: 1 hour(s), 26 minute(s), 14 second(s)
Memory Processes Infected: 0
Memory Modules Infected: 0
Registry Keys Infected: 3
Registry Values Infected: 1
Registry Data Items Infected: 0
Folders Infected: 2
Files Infected: 6
Memory Processes Infected:
(No malicious items detected)
Memory Modules Infected:
(No malicious items detected)
Registry Keys Infected:
HKEY_CLASSES_ROOT\CLSID\{ba603215-23f2-42ad-f4e4-00aac39caa53} (Trojan.Ertfor) -> Quarantined and deleted successfully.
HKEY_CURRENT_USER\SOFTWARE\Microsoft\Windows\CurrentVersion\Ext\Stats\{ba603215-23f2-42ad-f4e4-00aac39caa53} (Trojan.Ertfor) -> Quarantined and deleted successfully.
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\Browser Helper Objects\{ba603215-23f2-42ad-f4e4-00aac39caa53} (Trojan.Ertfor) -> Quarantined and deleted successfully.
Registry Values Infected:
HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\SharedTaskScheduler\{ba603215-23f2-42ad-f4e4-00aac39caa53} (Trojan.Ertfor) -> Quarantined and deleted successfully.
Registry Data Items Infected:
(No malicious items detected)
Folders Infected:
C:\RECYCLER\S-1-5-21-0243936033-3052116371-381863308-1811 (Trojan.Agent) -> Quarantined and deleted successfully.
C:\RECYCLER\s-1-5-21-0243936033-3052116371-381863308-1858 (Worm.Autorun) -> Quarantined and deleted successfully.
Files Infected:
C:\bqegh.exe (Trojan.Dropper) -> Quarantined and deleted successfully.
C:\Windows\System32\beep.sys (Rootkit.Agent) -> Quarantined and deleted successfully.
C:\Windows\System32\serfing.dll (Rootkit.Agent) -> Delete on reboot.
C:\Windows\System32\drivers\serfing.sys (Rootkit.Agent) -> Quarantined and deleted successfully.
C:\RECYCLER\S-1-5-21-0243936033-3052116371-381863308-1811\Desktop.ini (Trojan.Agent) -> Quarantined and deleted successfully.
C:\RECYCLER\s-1-5-21-0243936033-3052116371-381863308-1858\Desktop.ini (Worm.Autorun) -> Quarantined and deleted successfully.
^ permalink raw reply
* [PATCH] git branch -D: give a better error message when lockfile creation fails
From: Miklos Vajna @ 2009-09-26 23:15 UTC (permalink / raw)
To: Matthieu.Moy; +Cc: spearce, peff, git
In-Reply-To: <vpqy6o15v6m.fsf@bauges.imag.fr>
Previously the old error message just told the user that it was not
possible to delete the ref from the packed-refs file. Give instructions
on how to resolve the problem.
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
Changes since the previous one:
* unable_to_lock_index() renamed to unable_to_lock()
* NORETURN is back for unable_to_lock_index_die()
cache.h | 1 +
lockfile.c | 23 ++++++++++++++++++-----
refs.c | 4 +++-
3 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/cache.h b/cache.h
index 1a6412d..797cc4c 100644
--- a/cache.h
+++ b/cache.h
@@ -489,6 +489,7 @@ struct lock_file {
};
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
+extern int unable_to_lock(const char *path, int err, int noreturn);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);
diff --git a/lockfile.c b/lockfile.c
index eb931ed..3bacda4 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -156,17 +156,30 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
}
-NORETURN void unable_to_lock_index_die(const char *path, int err)
+int unable_to_lock(const char *path, int err, int noreturn)
{
+ struct strbuf buf = STRBUF_INIT;
+ int ret;
+
if (err == EEXIST) {
- die("Unable to create '%s.lock': %s.\n\n"
+ strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
"If no other git process is currently running, this probably means a\n"
"git process crashed in this repository earlier. Make sure no other git\n"
"process is running and remove the file manually to continue.",
path, strerror(err));
- } else {
- die("Unable to create '%s.lock': %s", path, strerror(err));
- }
+ } else
+ strbuf_addf(&buf, "Unable to create '%s.lock': %s", path, strerror(err));
+ if (noreturn)
+ die(buf.buf);
+ ret = error(buf.buf);
+ strbuf_release(&buf);
+ return ret;
+}
+
+NORETURN void unable_to_lock_index_die(const char *path, int err)
+{
+ unable_to_lock(path, err, 1);
+ die("unable_to_lock() should have died already");
}
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)
diff --git a/refs.c b/refs.c
index 24865cf..3d635ae 100644
--- a/refs.c
+++ b/refs.c
@@ -972,8 +972,10 @@ static int repack_without_ref(const char *refname)
if (!found)
return 0;
fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
- if (fd < 0)
+ if (fd < 0) {
+ unable_to_lock(git_path("packed-refs"), errno, 0);
return error("cannot delete '%s' from packed refs", refname);
+ }
for (list = packed_ref_list; list; list = list->next) {
char line[PATH_MAX + 100];
--
1.6.5.rc1.44.ga1675.dirty
^ 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