* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Giuseppe Scrivano @ 2009-09-27 8:21 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Jeff King, git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <alpine.LFD.2.00.0909262038470.4997@xanadu.home>
Nicolas Pitre <nico@fluxnic.net> writes:
> And the compiler (at least gcc) is indeed smart enough to realize that
> nothing uses the result from the last statement, and does optimize away
> the code associated to it already. So this patch is unlikely to change
> anything to the compiled result.
Right, and gcc can do many other amazing things. But still it is used a
variable that is never accessed, removing it can make the code slightly
more readable.
Cheers,
Giuseppe
^ permalink raw reply
* Re: [PATCH] git branch -D: give a better error message when lockfile creation fails
From: Jeff King @ 2009-09-27 8:21 UTC (permalink / raw)
To: Miklos Vajna; +Cc: Matthieu.Moy, spearce, git
In-Reply-To: <1254006909-1862-1-git-send-email-vmiklos@frugalware.org>
On Sun, Sep 27, 2009 at 01:15:09AM +0200, Miklos Vajna wrote:
> Changes since the previous one:
>
> * unable_to_lock_index() renamed to unable_to_lock()
> * NORETURN is back for unable_to_lock_index_die()
Much better, but:
> + if (noreturn)
> + die(buf.buf);
> + ret = error(buf.buf);
These need to be:
die("%s", buf.buf);
as the resulting message (which contains the filename) may have '%' in
it.
> +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");
> }
Maybe it is just me, but that extra die() that should never be reached
is terribly ugly. I would do it with two functions, one that dies and
one that doesn't, with a helper to format the message. IOW, this:
-- >8 --
From: Miklos Vajna <vmiklos@frugalware.org>
Subject: [PATCH] git branch -D: give a better error message when lockfile creation fails
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>
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 1 +
lockfile.c | 26 ++++++++++++++++++++------
refs.c | 4 +++-
3 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/cache.h b/cache.h
index 1a6412d..a5eeead 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_error(const char *path, int err);
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..6851fa5 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -155,18 +155,32 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)
return lk->fd;
}
-
-NORETURN void unable_to_lock_index_die(const char *path, int err)
+static char *unable_to_lock_message(const char *path, int err)
{
+ struct strbuf buf = STRBUF_INIT;
+
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));
+ return strbuf_detach(&buf, NULL);
+}
+
+int unable_to_lock_error(const char *path, int err)
+{
+ char *msg = unable_to_lock_message(path, err);
+ error("%s", msg);
+ free(msg);
+ return -1;
+}
+
+NORETURN void unable_to_lock_index_die(const char *path, int err)
+{
+ die("%s", unable_to_lock_message(path, err));
}
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..808f56b 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_error(git_path("packed-refs"), errno);
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.rc2.197.g25cf3
^ permalink raw reply related
* Re: 'git branch --no-merge' is ambiguous
From: Andreas Schwab @ 2009-09-27 8:01 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Pierre Habouzit, Git Mailing List
In-Reply-To: <20090927073305.GA15393@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> -- >8 --
> From: Andreas Schwab <schwab@linux-m68k.org>
> Subject: [PATCH] parse-opt: ignore negation of OPT_NONEG for ambiguity checks
>
> parse_long_opt always matches both --opt and --no-opt for any option
> "opt", and only get_value checks whether --no-opt is actually valid.
> Since the options for git branch contains both "no-merged" and "merged"
> there are two matches for --no-merge, but no exact match. With this
> patch the negation of a NONEG option is rejected earlier, but it changes
> the error message from "option `no-opt' isn't available" to "unknown
> option `no-opt'".
>
> [jk: added test]
>
> Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Andreas Schwab <schwab@linux-m68k.org>
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* [PATCH] improve reflog date/number heuristic
From: Jeff King @ 2009-09-27 7:50 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Alf Kristian St??yle, git
In-Reply-To: <20090925222920.GZ14660@spearce.org>
When we show a reflog, we have two ways of naming the entry:
by sequence number (e.g., HEAD@{0}) or by date (e.g.,
HEAD@{10 minutes ago}). There is no explicit option to set
one or the other, but we guess based on whether or not the
user has provided us with a date format, showing them the
date version if they have done so, and the sequence number
otherwise.
This usually made sense if the use did something like "git
log -g --date=relative". However, it didn't make much sense
if the user set the date format using the log.date config
variable; in that case, all of their reflogs would end up as
dates.
This patch records the source of the date format and only
triggers the date-based view if --date= was given on the
command line.
Signed-off-by: Jeff King <peff@peff.net>
---
On Fri, Sep 25, 2009 at 03:29:20PM -0700, Shawn O. Pearce wrote:
> I agree. I doubt anyone is relying on log.date to reformat the
> output of `git reflog show` or `git stash list`, so this is probably
> a reasonable change to make. Even if they were trying to use that,
> its a bug.
>
> Care to wrap this up in a patch?
Here it is.
log-tree.c | 4 +++-
revision.c | 2 ++
revision.h | 3 ++-
3 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 1c9eefe..1618f3c 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -390,7 +390,9 @@ void show_log(struct rev_info *opt)
*/
show_reflog_message(opt->reflog_info,
opt->commit_format == CMIT_FMT_ONELINE,
- opt->date_mode);
+ opt->date_mode_explicit ?
+ opt->date_mode :
+ DATE_NORMAL);
if (opt->commit_format == CMIT_FMT_ONELINE)
return;
}
diff --git a/revision.c b/revision.c
index 35eca4a..9fc4e8d 100644
--- a/revision.c
+++ b/revision.c
@@ -1159,8 +1159,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->simplify_history = 0;
} else if (!strcmp(arg, "--relative-date")) {
revs->date_mode = DATE_RELATIVE;
+ revs->date_mode_explicit = 1;
} else if (!strncmp(arg, "--date=", 7)) {
revs->date_mode = parse_date_format(arg + 7);
+ revs->date_mode_explicit = 1;
} else if (!strcmp(arg, "--log-size")) {
revs->show_log_size = 1;
}
diff --git a/revision.h b/revision.h
index 9d0dddb..b6421a6 100644
--- a/revision.h
+++ b/revision.h
@@ -81,7 +81,8 @@ struct rev_info {
show_merge:1,
abbrev_commit:1,
use_terminator:1,
- missing_newline:1;
+ missing_newline:1,
+ date_mode_explicit:1;
enum date_mode date_mode;
unsigned int abbrev;
--
1.6.5.rc2.197.g25cf3
^ permalink raw reply related
* Re: [PATCH] git-am: force egrep to use correct characters set
From: Jeff King @ 2009-09-27 7:40 UTC (permalink / raw)
To: Christian Himpel; +Cc: git
In-Reply-To: <f0bd48168975c3b2328cf26f9a37a0f54b898473.1253896646.git.chressie@gmail.com>
On Fri, Sep 25, 2009 at 06:43:20PM +0200, Christian Himpel wrote:
> According to egrep(1) the US-ASCII table is used when LC_ALL=C is set.
> We do not rely here on the LC_ALL value we get from the environment.
Hmm. Probably makes sense here, as it is a wide enough range that it may
pick up other stray non-ascii characters in other charsets (though as
the manpage notes, the likely thing is to pick up A-Z along with a-z,
which is OK here as we encompass both in our range).
There are two other calls to egrep with brackets (both in
git-submodule.sh), but they are just [0-7], which is presumably OK in
just about any charset.
Do you happen to know a charset in which this is a problem, just for
reference?
-Peff
^ permalink raw reply
* Re: 'git branch --no-merge' is ambiguous
From: Jeff King @ 2009-09-27 7:33 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Johannes Sixt, Pierre Habouzit, Git Mailing List
In-Reply-To: <m2ljk250f7.fsf@igel.home>
On Fri, Sep 25, 2009 at 08:44:44PM +0200, Andreas Schwab wrote:
> parse_long_opt always matches both --opt and --no-opt for any option
> "opt", and only get_value checks whether --no-opt is actually valid.
> Since the options for git branch contains both "no-merged" and "merged"
> there are two matches for --no-merge, but no exact match. With this
> patch the negation of a NONEG option is rejected earlier, but it changes
> the error message from "option `no-opt' isn't available" to "unknown
> option `no-opt'".
Thanks. Reading through the code, I came to the same conclusion: we
shouldn't be looking at --no-* at all if we are NONEG. I think the
change in error message is acceptable.
It is a little bit annoying that builtin-branch needs to have this as
two separate options in the first place. But it wants to be able to do
--no-merged with an argument, which is not currently possible with just
a negation of --merged. I don't know if it is worth adding an
OPT_NEGARG.
Below is what I'm going to commit. Can I get your Signed-off-by?
-- >8 --
From: Andreas Schwab <schwab@linux-m68k.org>
Subject: [PATCH] parse-opt: ignore negation of OPT_NONEG for ambiguity checks
parse_long_opt always matches both --opt and --no-opt for any option
"opt", and only get_value checks whether --no-opt is actually valid.
Since the options for git branch contains both "no-merged" and "merged"
there are two matches for --no-merge, but no exact match. With this
patch the negation of a NONEG option is rejected earlier, but it changes
the error message from "option `no-opt' isn't available" to "unknown
option `no-opt'".
[jk: added test]
Signed-off-by: Jeff King <peff@peff.net>
---
parse-options.c | 3 +++
t/t0040-parse-options.sh | 20 ++++++++++++++++++++
test-parse-options.c | 5 +++++
3 files changed, 28 insertions(+), 0 deletions(-)
diff --git a/parse-options.c b/parse-options.c
index a64a4d6..f559411 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -230,6 +230,9 @@ is_abbreviated:
abbrev_flags = flags;
continue;
}
+ /* negation allowed? */
+ if (options->flags & PARSE_OPT_NONEG)
+ continue;
/* negated and abbreviated very much? */
if (!prefixcmp("no-", arg)) {
flags |= OPT_UNSET;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index bbc821e..3d450ed 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -33,6 +33,8 @@ Magic arguments
--quux means --quux
-NUM set integer to NUM
+ same as -b
+ --ambiguous positive ambiguity
+ --no-ambiguous negative ambiguity
Standard options
--abbrev[=<n>] use <n> digits to display SHA-1s
@@ -315,4 +317,22 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
test_cmp expect output
'
+cat >expect <<EOF
+boolean: 0
+integer: 0
+timestamp: 0
+string: (not set)
+abbrev: 7
+verbose: 0
+quiet: no
+dry run: no
+file: (not set)
+EOF
+
+test_expect_success 'negation of OPT_NONEG flags is not ambiguous' '
+ test-parse-options --no-ambig >output 2>output.err &&
+ test ! -s output.err &&
+ test_cmp expect output
+'
+
test_done
diff --git a/test-parse-options.c b/test-parse-options.c
index efa734b..acd1a2b 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -8,6 +8,7 @@ static int abbrev = 7;
static int verbose = 0, dry_run = 0, quiet = 0;
static char *string = NULL;
static char *file = NULL;
+static int ambiguous;
static int length_callback(const struct option *opt, const char *arg, int unset)
{
@@ -59,6 +60,10 @@ int main(int argc, const char **argv)
number_callback),
{ OPTION_BOOLEAN, '+', NULL, &boolean, NULL, "same as -b",
PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_NODASH },
+ { OPTION_BOOLEAN, 0, "ambiguous", &ambiguous, NULL,
+ "positive ambiguity", PARSE_OPT_NOARG | PARSE_OPT_NONEG },
+ { OPTION_BOOLEAN, 0, "no-ambiguous", &ambiguous, NULL,
+ "negative ambiguity", PARSE_OPT_NOARG | PARSE_OPT_NONEG },
OPT_GROUP("Standard options"),
OPT__ABBREV(&abbrev),
OPT__VERBOSE(&verbose),
--
1.6.5.rc2.197.g25cf3
^ permalink raw reply related
* Re: git clone sending unneeded objects
From: Jason Merrill @ 2009-09-27 4:35 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Nicolas Pitre, Matthieu Moy, git, Hin-Tak Leung
In-Reply-To: <20090927020409.GK14660@spearce.org>
On 09/26/2009 10:04 PM, Shawn O. Pearce wrote:
> Actually, if those refs have not changed, quickfetch should kick in
> and realize that all 410610 objects are reachable locally without
> errors, permitting the client to avoid the object transfer.
>
> However, if *ANY* of those refs were to change to something you
> don't actually have, quickfetch would fail, and we would need to
> fetch all 410610 objects.
Right. That seems unfortunate to me; couldn't fetch do a bit more
checking before it decides to download the whole world again?
Jason
^ permalink raw reply
* Re: git clone sending unneeded objects
From: Nicolas Pitre @ 2009-09-27 2:31 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Jason Merrill, Matthieu Moy, git, Hin-Tak Leung
In-Reply-To: <20090927020409.GK14660@spearce.org>
On Sat, 26 Sep 2009, Shawn O. Pearce wrote:
> Nicolas Pitre <nico@fluxnic.net> wrote:
> > And even if the broken clone (before my patch) did pull everything from
> > gcc.git, in the cloned repository those 410610 extra objects are
> > considered as garbage because nothing actually reference them. So even
> > if you decide to fetch the extra branches that the initial clone didn't
> > pick up, or if you do reference that repository with "garbage" objects
> > for another clone to which you want to add those extra branches, git has
> > no way to know that it already had access to those objects locally and
> > "ungarbage" them as they aren't referenced. Result is a useless fetch
> > of 410610 objects that you already have, but that you weren't supposed
> > to have in the first place.
>
> Just to clarify a minor nit:
>
> Actually, if those refs have not changed, quickfetch should kick in
> and realize that all 410610 objects are reachable locally without
> errors, permitting the client to avoid the object transfer.
>
> However, if *ANY* of those refs were to change to something you
> don't actually have, quickfetch would fail, and we would need to
> fetch all 410610 objects.
Right. But since we're talking about a git mirror for the gcc svn repo
and gcc is a rather active project, the likelyhood of any ref to change
at any time is rather high.
Nicolas
^ permalink raw reply
* Re: git clone sending unneeded objects
From: Nicolas Pitre @ 2009-09-27 2:26 UTC (permalink / raw)
To: Jason Merrill; +Cc: Matthieu Moy, git, Hin-Tak Leung
In-Reply-To: <4ABE1818.6010007@redhat.com>
On Sat, 26 Sep 2009, Jason Merrill wrote:
> On 09/26/2009 12:44 AM, Jason Merrill wrote:
> > git config remote.origin.fetch 'refs/remotes/*:refs/remotes/origin/*'
> > git fetch
>
> git count-objects -v before:
>
> count: 44
> size: 1768
> in-pack: 1399509
> packs: 1
> size-pack: 600456
> prune-packable: 0
> garbage: 0
I'm sure if you had done 'git rev-list --all --objects | wc -l' at that
point, the result would have been something around 900000. That's the
actual number of objects git had a reference to, compared to the total
objects contained in the object store.
> and after (transferred 278MB):
>
> count: 44
> size: 1768
> in-pack: 1947339
> packs: 2
> size-pack: 1178408
> prune-packable: 8
> garbage: 0
And those 500000 extra objects or so (minus a couple dozens which were
probably used to "complete" the fetched thin pack and are duplicates of
local objects -- the fetch progress message gave the exact number) were
obtained from the remote repository because git has no way to tell the
remote it already had them. That's what I was explaining in my previous
email.
> and then after git gc --prune=now:
>
> count: 0
> size: 0
> in-pack: 1399613
> packs: 1
> size-pack: 839900
> prune-packable: 0
> garbage: 0
>
> So I only actually needed 104 more objects, but fetch wasn't clever enough to
> see that, and my new pack is much less efficient.
Like I said, it's not that the fetch wasn't clever enough. Rather that
your initial clone asked for way too many objects in the first place.
That's what my patch fixed.
Now the pack efficiency can be explained as well. A single pack is
always going to be more efficient than 2 packs. Problem is when you do
a gc, by default git does the least costly operation which consists of
copying as much data from existing packs without extra processing.
That means that many objects were copied from the second (newly
received) pack although a better delta representation was most probably
available in the other larger pack (remember that most objects from that
second pack already existed in the first pack). Git do select the
second pack in preference to the other pack because it is more recent,
and normally more recent packs contains more recent objects which is a
good heuristic to optimizes the object enumeration. In this case this
didn't produce a good result, but again we're talking about a scenario
which is bogus from the start and shouldn't be.
So if you do a 'git gc --aggressive' and let it run for a while, you
should get back a smaller pack, possibly even much smaller than the
original
one.
Nicolas
^ permalink raw reply
* Re: git clone sending unneeded objects
From: Shawn O. Pearce @ 2009-09-27 2:04 UTC (permalink / raw)
To: Nicolas Pitre; +Cc: Jason Merrill, Matthieu Moy, git, Hin-Tak Leung
In-Reply-To: <alpine.LFD.2.00.0909262059520.4997@xanadu.home>
Nicolas Pitre <nico@fluxnic.net> wrote:
> And even if the broken clone (before my patch) did pull everything from
> gcc.git, in the cloned repository those 410610 extra objects are
> considered as garbage because nothing actually reference them. So even
> if you decide to fetch the extra branches that the initial clone didn't
> pick up, or if you do reference that repository with "garbage" objects
> for another clone to which you want to add those extra branches, git has
> no way to know that it already had access to those objects locally and
> "ungarbage" them as they aren't referenced. Result is a useless fetch
> of 410610 objects that you already have, but that you weren't supposed
> to have in the first place.
Just to clarify a minor nit:
Actually, if those refs have not changed, quickfetch should kick in
and realize that all 410610 objects are reachable locally without
errors, permitting the client to avoid the object transfer.
However, if *ANY* of those refs were to change to something you
don't actually have, quickfetch would fail, and we would need to
fetch all 410610 objects.
--
Shawn.
^ permalink raw reply
* Re: git clone sending unneeded objects
From: Nicolas Pitre @ 2009-09-27 1:27 UTC (permalink / raw)
To: Jason Merrill; +Cc: Matthieu Moy, git, Hin-Tak Leung
In-Reply-To: <4ABD9C2C.60800@redhat.com>
On Sat, 26 Sep 2009, Jason Merrill wrote:
> Incidentally, somewhat related to this issue, I've noticed that if I fetch a
> branch which I don't currently have in my repository, and I have most of the
> commits on that branch in my object store (or in an alternate repository) but
> not the most recent commit, git fetch isn't smart enough to only grab the
> commits I'm actually missing, it wants to fetch much more.
>
> I would expect that since the clone pulled down everything in the gcc.git
> repository, I could then do
>
> git config remote.origin.fetch 'refs/remotes/*:refs/remotes/origin/*'
> git fetch
>
> and have all the branches, not just the ones in refs/heads. But when I do
> this git fetch wants to fetch some 500k redundant objects.
Well... Assuming a fixed git using the patch I posted yesterday, my
clone of gcc.git has 988941 objects. The source repository used for the
clone has 1399551 objects. Of course the source repo has more objects
because it has extra branches in the refs/remotes/ namespace that the
clone didn't fetch. If you wish to also fetch those branches as you
illustrated above then you'll get the difference i.e. 410610 additional
objects.
And even if the broken clone (before my patch) did pull everything from
gcc.git, in the cloned repository those 410610 extra objects are
considered as garbage because nothing actually reference them. So even
if you decide to fetch the extra branches that the initial clone didn't
pick up, or if you do reference that repository with "garbage" objects
for another clone to which you want to add those extra branches, git has
no way to know that it already had access to those objects locally and
"ungarbage" them as they aren't referenced. Result is a useless fetch
of 410610 objects that you already have, but that you weren't supposed
to have in the first place.
Nicolas
^ permalink raw reply
* Re: [PATCH] Remove various dead assignments and dead increments found by the clang static analyzer
From: Nicolas Pitre @ 2009-09-27 0:41 UTC (permalink / raw)
To: Jeff King; +Cc: Giuseppe Scrivano, git, Johannes Schindelin, Sverre Rabbelier
In-Reply-To: <20090926204604.GA2960@coredump.intra.peff.net>
On Sat, 26 Sep 2009, Jeff King wrote:
> 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.
And the compiler (at least gcc) is indeed smart enough to realize that
nothing uses the result from the last statement, and does optimize away
the code associated to it already. So this patch is unlikely to change
anything to the compiled result.
Nicolas
^ permalink raw reply
* Re: [PATCH] make 'git clone' ask the remote only for objects it cares about
From: Nicolas Pitre @ 2009-09-27 0:26 UTC (permalink / raw)
To: Shawn O. Pearce
Cc: Junio C Hamano, Jason Merrill, Matthieu Moy, git, Hin-Tak Leung
In-Reply-To: <20090926195039.GG14660@spearce.org>
On Sat, 26 Sep 2009, Shawn O. Pearce wrote:
> 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.
Yeah, I noticed. Since I didn't know what was the original intent for
it, I just left it there.
Nicolas
^ permalink raw reply
* [PATCH] gitweb: fix spelling errors in comments
From: Miklos Vajna @ 2009-09-26 23:32 UTC (permalink / raw)
To: git
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
gitweb/gitweb.perl | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
The plural form of "id" is "ids", not "id's", right?
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 24b2193..b66c4d0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4949,7 +4949,7 @@ sub git_blob_plain {
die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
- # blobs defined by non-textual hash id's can be cached
+ # blobs defined by non-textual hash ids can be cached
$expires = "+1d";
}
@@ -5001,7 +5001,7 @@ sub git_blob {
die_error(400, "No file name defined");
}
} elsif ($hash =~ m/^[0-9a-fA-F]{40}$/) {
- # blobs defined by non-textual hash id's can be cached
+ # blobs defined by non-textual hash ids can be cached
$expires = "+1d";
}
@@ -5346,7 +5346,7 @@ sub git_commit {
@difftree = map { chomp; $_ } <$fd>;
close $fd or die_error(404, "Reading git-diff-tree failed");
- # non-textual hash id's can be cached
+ # non-textual hash ids can be cached
my $expires;
if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
$expires = "+1d";
@@ -5513,7 +5513,7 @@ sub git_blobdiff {
$hash_parent ||= $diffinfo{'from_id'};
$hash ||= $diffinfo{'to_id'};
- # non-textual hash id's can be cached
+ # non-textual hash ids can be cached
if ($hash_base =~ m/^[0-9a-fA-F]{40}$/ &&
$hash_parent_base =~ m/^[0-9a-fA-F]{40}$/) {
$expires = '+1d';
@@ -5733,7 +5733,7 @@ sub git_commitdiff {
die_error(400, "Unknown commitdiff format");
}
- # non-textual hash id's can be cached
+ # non-textual hash ids can be cached
my $expires;
if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
$expires = "+1d";
--
1.6.5.rc1.44.ga1675.dirty
^ permalink raw reply related
* [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
* 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
* 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: [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: 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: 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: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: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 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: 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] 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
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