* [PATCH 3/3] bulk-checkin: do not write an object that already exists
From: Junio C Hamano @ 2011-11-18 7:11 UTC (permalink / raw)
To: git
In-Reply-To: <1321600317-30546-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
bulk-checkin.c | 12 +++++++++++-
t/t1050-large.sh | 18 +++++++++++++++++-
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 82166ba..60178ef 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -29,7 +29,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
if (!state->f)
return;
- if (state->nr_written == 1) {
+ if (state->nr_written == 0) {
+ close(state->f->fd);
+ unlink(state->pack_tmp_name);
+ goto clear_exit;
+ } else if (state->nr_written == 1) {
sha1close(state->f, sha1, CSUM_FSYNC);
} else {
int fd = sha1close(state->f, sha1, 0);
@@ -45,6 +49,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
&state->pack_idx_opts, sha1);
for (i = 0; i < state->nr_written; i++)
free(state->written[i]);
+
+clear_exit:
free(state->written);
memset(state, 0, sizeof(*state));
@@ -56,6 +62,10 @@ static int already_written(struct bulk_checkin_state *state, unsigned char sha1[
{
int i;
+ /* The object may already exist in the repository */
+ if (has_sha1_file(sha1))
+ return 1;
+
/* Might want to keep the list sorted */
for (i = 0; i < state->nr_written; i++)
if (!hashcmp(state->written[i]->sha1, sha1))
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index fbd5ced..0726472 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -9,6 +9,7 @@ test_expect_success setup '
git config core.bigfilethreshold 200k &&
echo X | dd of=large1 bs=1k seek=2000 &&
echo X | dd of=large2 bs=1k seek=2000 &&
+ echo X | dd of=large3 bs=1k seek=2000 &&
echo Y | dd of=huge bs=1k seek=2500
'
@@ -34,7 +35,22 @@ test_expect_success 'add a large file or two' '
test -f "$l" || continue
bad=t
done &&
- test -z "$bad"
+ test -z "$bad" &&
+
+ # attempt to add another copy of the same
+ git add large3 &&
+ bad= count=0 &&
+ for p in .git/objects/pack/pack-*.pack
+ do
+ count=$(( $count + 1 ))
+ if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
+ then
+ continue
+ fi
+ bad=t
+ done &&
+ test -z "$bad" &&
+ test $count = 1
'
test_expect_success 'checkout a large file' '
--
1.7.8.rc3.111.g7d421
^ permalink raw reply related
* [PATCH 1/3] csum-file: introduce sha1file_checkpoint
From: Junio C Hamano @ 2011-11-18 7:11 UTC (permalink / raw)
To: git
In-Reply-To: <1321600317-30546-1-git-send-email-gitster@pobox.com>
It is useful to be able to rewind a check-summed file to a certain
previous state after writing data into it using sha1write() API. The
fast-import command does this after streaming a blob data to the packfile
being generated and then noticing that the same blob has already been
written, and it does this with a private code truncate_pack() that is
commented as "Yes, this is a layering violation".
Introduce two API functions, sha1file_checkpoint(), that allows the caller
to save a state of a sha1file, and then later revert it to the saved state.
Use it to reimplement truncate_pack().
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
csum-file.c | 20 ++++++++++++++++++++
csum-file.h | 9 +++++++++
fast-import.c | 25 ++++++++-----------------
3 files changed, 37 insertions(+), 17 deletions(-)
diff --git a/csum-file.c b/csum-file.c
index fc97d6e..53f5375 100644
--- a/csum-file.c
+++ b/csum-file.c
@@ -158,6 +158,26 @@ struct sha1file *sha1fd_throughput(int fd, const char *name, struct progress *tp
return f;
}
+void sha1file_checkpoint(struct sha1file *f, struct sha1file_checkpoint *checkpoint)
+{
+ sha1flush(f);
+ checkpoint->offset = f->total;
+ checkpoint->ctx = f->ctx;
+}
+
+int sha1file_truncate(struct sha1file *f, struct sha1file_checkpoint *checkpoint)
+{
+ off_t offset = checkpoint->offset;
+
+ if (ftruncate(f->fd, offset) ||
+ lseek(f->fd, offset, SEEK_SET) != offset)
+ return -1;
+ f->total = offset;
+ f->ctx = checkpoint->ctx;
+ f->offset = 0; /* sha1flush() was called in checkpoint */
+ return 0;
+}
+
void crc32_begin(struct sha1file *f)
{
f->crc32 = crc32(0, NULL, 0);
diff --git a/csum-file.h b/csum-file.h
index 6a7967c..3b540bd 100644
--- a/csum-file.h
+++ b/csum-file.h
@@ -17,6 +17,15 @@ struct sha1file {
unsigned char buffer[8192];
};
+/* Checkpoint */
+struct sha1file_checkpoint {
+ off_t offset;
+ git_SHA_CTX ctx;
+};
+
+extern void sha1file_checkpoint(struct sha1file *, struct sha1file_checkpoint *);
+extern int sha1file_truncate(struct sha1file *, struct sha1file_checkpoint *);
+
/* sha1close flags */
#define CSUM_CLOSE 1
#define CSUM_FSYNC 2
diff --git a/fast-import.c b/fast-import.c
index 8d8ea3c..a8db41b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1143,17 +1143,11 @@ static int store_object(
return 0;
}
-static void truncate_pack(off_t to, git_SHA_CTX *ctx)
+static void truncate_pack(struct sha1file_checkpoint *checkpoint)
{
- if (ftruncate(pack_data->pack_fd, to)
- || lseek(pack_data->pack_fd, to, SEEK_SET) != to)
+ if (sha1file_truncate(pack_file, checkpoint))
die_errno("cannot truncate pack to skip duplicate");
- pack_size = to;
-
- /* yes this is a layering violation */
- pack_file->total = to;
- pack_file->offset = 0;
- pack_file->ctx = *ctx;
+ pack_size = checkpoint->offset;
}
static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)
@@ -1166,8 +1160,8 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)
unsigned long hdrlen;
off_t offset;
git_SHA_CTX c;
- git_SHA_CTX pack_file_ctx;
git_zstream s;
+ struct sha1file_checkpoint checkpoint;
int status = Z_OK;
/* Determine if we should auto-checkpoint. */
@@ -1175,11 +1169,8 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)
|| (pack_size + 60 + len) < pack_size)
cycle_packfile();
- offset = pack_size;
-
- /* preserve the pack_file SHA1 ctx in case we have to truncate later */
- sha1flush(pack_file);
- pack_file_ctx = pack_file->ctx;
+ sha1file_checkpoint(pack_file, &checkpoint);
+ offset = checkpoint.offset;
hdrlen = snprintf((char *)out_buf, out_sz, "blob %" PRIuMAX, len) + 1;
if (out_sz <= hdrlen)
@@ -1245,14 +1236,14 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)
if (e->idx.offset) {
duplicate_count_by_type[OBJ_BLOB]++;
- truncate_pack(offset, &pack_file_ctx);
+ truncate_pack(&checkpoint);
} else if (find_sha1_pack(sha1, packed_git)) {
e->type = OBJ_BLOB;
e->pack_id = MAX_PACK_ID;
e->idx.offset = 1; /* just not zero! */
duplicate_count_by_type[OBJ_BLOB]++;
- truncate_pack(offset, &pack_file_ctx);
+ truncate_pack(&checkpoint);
} else {
e->depth = 0;
--
1.7.8.rc3.111.g7d421
^ permalink raw reply related
* [PATCH 2/3] bulk-checkin: do not write the same object twice
From: Junio C Hamano @ 2011-11-18 7:11 UTC (permalink / raw)
To: git
In-Reply-To: <1321600317-30546-1-git-send-email-gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
bulk-checkin.c | 28 ++++++++++++++++++++++++----
t/t1050-large.sh | 20 +++++++++++++-------
2 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index c7e693e..82166ba 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -52,6 +52,17 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state)
reprepare_packed_git();
}
+static int already_written(struct bulk_checkin_state *state, unsigned char sha1[])
+{
+ int i;
+
+ /* Might want to keep the list sorted */
+ for (i = 0; i < state->nr_written; i++)
+ if (!hashcmp(state->written[i]->sha1, sha1))
+ return 1;
+ return 0;
+}
+
static void deflate_to_pack(struct bulk_checkin_state *state,
unsigned char sha1[],
int fd, size_t size, enum object_type type,
@@ -64,6 +75,7 @@ static void deflate_to_pack(struct bulk_checkin_state *state,
int write_object = (flags & HASH_WRITE_OBJECT);
int status = Z_OK;
struct pack_idx_entry *idx = NULL;
+ struct sha1file_checkpoint checkpoint;
hdrlen = sprintf((char *)obuf, "%s %" PRIuMAX,
typename(type), (uintmax_t)size) + 1;
@@ -73,6 +85,7 @@ static void deflate_to_pack(struct bulk_checkin_state *state,
if (write_object) {
idx = xcalloc(1, sizeof(*idx));
idx->offset = state->offset;
+ sha1file_checkpoint(state->f, &checkpoint);
crc32_begin(state->f);
}
memset(&s, 0, sizeof(s));
@@ -121,10 +134,17 @@ static void deflate_to_pack(struct bulk_checkin_state *state,
git_SHA1_Final(sha1, &ctx);
if (write_object) {
idx->crc32 = crc32_end(state->f);
- hashcpy(idx->sha1, sha1);
- ALLOC_GROW(state->written,
- state->nr_written + 1, state->alloc_written);
- state->written[state->nr_written++] = idx;
+
+ if (already_written(state, sha1)) {
+ sha1file_truncate(state->f, &checkpoint);
+ state->offset = checkpoint.offset;
+ free(idx);
+ } else {
+ hashcpy(idx->sha1, sha1);
+ ALLOC_GROW(state->written,
+ state->nr_written + 1, state->alloc_written);
+ state->written[state->nr_written++] = idx;
+ }
}
}
diff --git a/t/t1050-large.sh b/t/t1050-large.sh
index 36def25..fbd5ced 100755
--- a/t/t1050-large.sh
+++ b/t/t1050-large.sh
@@ -7,22 +7,28 @@ test_description='adding and checking out large blobs'
test_expect_success setup '
git config core.bigfilethreshold 200k &&
- echo X | dd of=large bs=1k seek=2000 &&
+ echo X | dd of=large1 bs=1k seek=2000 &&
+ echo X | dd of=large2 bs=1k seek=2000 &&
echo Y | dd of=huge bs=1k seek=2500
'
test_expect_success 'add a large file or two' '
- git add large huge &&
+ git add large1 huge large2 &&
# make sure we got a single packfile and no loose objects
- bad= count=0 &&
+ bad= count=0 idx= &&
for p in .git/objects/pack/pack-*.pack
do
count=$(( $count + 1 ))
- test -f "$p" && continue
+ if test -f "$p" && idx=${p%.pack}.idx && test -f "$idx"
+ then
+ continue
+ fi
bad=t
done &&
test -z "$bad" &&
test $count = 1 &&
+ cnt=$(git show-index <"$idx" | wc -l) &&
+ test $cnt = 2 &&
for l in .git/objects/??/??????????????????????????????????????
do
test -f "$l" || continue
@@ -32,10 +38,10 @@ test_expect_success 'add a large file or two' '
'
test_expect_success 'checkout a large file' '
- large=$(git rev-parse :large) &&
- git update-index --add --cacheinfo 100644 $large another &&
+ large1=$(git rev-parse :large1) &&
+ git update-index --add --cacheinfo 100644 $large1 another &&
git checkout another &&
- cmp large another ;# this must not be test_cmp
+ cmp large1 another ;# this must not be test_cmp
'
test_done
--
1.7.8.rc3.111.g7d421
^ permalink raw reply related
* [PATCH 0/3] bulk-checkin continued
From: Junio C Hamano @ 2011-11-18 7:11 UTC (permalink / raw)
To: git
In-Reply-To: <1319846051-462-1-git-send-email-gitster@pobox.com>
This updates the earlier bulk-checkin series ($gmane/184440) to further
enhance the "large file" topic from 1.7.6 cycle.
The first one adds two API functions to allow truncating a checksummed
file that is being written. This is the same patch as the one I sent
earlier today.
The second one prevents the bulk-checkin code from writing the same object
twice to the stream, and the third one further makes it notice that the
object it has just written already exists in the repository. In either
case, the packfile is rewound using the new sha1file_checkpoint/truncate
API, and the packfile itself is removed if truncation results in an empty
output.
The next step is to add the "split-blob" entry in the packfile, but that
is a much larger task and will take longer.
Junio C Hamano (3):
csum-file: introduce sha1file_checkpoint
bulk-checkin: do not write the same object twice
bulk-checkin: do not write an object that already exists
bulk-checkin.c | 40 +++++++++++++++++++++++++++++++++++-----
csum-file.c | 20 ++++++++++++++++++++
csum-file.h | 9 +++++++++
fast-import.c | 25 ++++++++-----------------
t/t1050-large.sh | 38 ++++++++++++++++++++++++++++++--------
5 files changed, 102 insertions(+), 30 deletions(-)
--
1.7.8.rc3.111.g7d421
^ permalink raw reply
* Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()
From: Jonathan Nieder @ 2011-11-18 6:52 UTC (permalink / raw)
To: Johan Herland
Cc: Nguyen Thai Ngoc Duy, Jeff King, Ramkumar Ramachandra, git,
Junio C Hamano, Michael Haggerty
In-Reply-To: <CALKQrgfTKmSd8se3n3xq89SXRmNPm3qz3Ckv2mUghot8kStKxA@mail.gmail.com>
Johan Herland wrote:
> Acked-by: Johan Herland <johan@herland.net>
Thanks for looking it over.
^ permalink raw reply
* Re: [PATCH] Makefile: add option to disable automatic dependency generation
From: Jonathan Nieder @ 2011-11-18 6:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nguyen Thai Ngoc Duy, Git Mailing List, Fredrik Kuivinen
In-Reply-To: <7vty62klg9.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> Eek. At least at the end user UI level, couldn't we do this as a tristate?
> E.g. "YesPlease" (or anything that begins with Y if you are ambitious) to
> explicitly enable, empty (or "auto") to autodetect, and anything else to
> decline?
Ah, I didn't mind the UI so much. Handling
COMPUTE_HEADER_DEPENDENCIES_FORCE = (yes | no | auto)
should be doable. I'd suggest making any other value error out, so
typos don't result in mysterious behavior.
> Even better, couldn't we either (1) rearrange .dep/ files somehow, so that
> compiler difference does not matter
Yes, I'm working on an incantation all the compilers like (it
shouldn't be hard --- adding an -MQ option should be enough, but I
want to understand the bug first). But even with such a fix, I think
it will be important to have a way to turn the feature off. When
someone using a compiler without -MMD support reports a bug, wouldn't
it be nice to be able to reproduce it?
^ permalink raw reply
* Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()
From: Johan Herland @ 2011-11-18 6:16 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Nguyen Thai Ngoc Duy, Jeff King, Ramkumar Ramachandra, git,
Junio C Hamano, Michael Haggerty
In-Reply-To: <20111118012746.GA22485@elie.hsd1.il.comcast.net>
On Fri, Nov 18, 2011 at 02:27, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Subject: notes merge: eliminate OUTPUT macro
>
> The macro is variadic, which breaks support for pre-C99 compilers,
> and it hides an "if", which can make code hard to understand on
> first reading if some arguments have side-effects.
>
> The OUTPUT macro seems to have been inspired by the "output" function
> from merge-recursive. But that function in merge-recursive exists to
> indent output based on the level of recursion and there is no similar
> justification for such a function in "notes merge".
>
> Noticed with 'make CC="gcc -std=c89 -pedantic"':
>
> notes-merge.c:24:22: warning: anonymous variadic macros were introduced in C99 [-Wvariadic-macros]
>
> Encouraged-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Acked-by: Johan Herland <johan@herland.net>
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [PATCH] Makefile: add option to disable automatic dependency generation
From: Junio C Hamano @ 2011-11-18 6:12 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Nguyen Thai Ngoc Duy, Git Mailing List, Fredrik Kuivinen
In-Reply-To: <20111118045742.GA25145@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Duy noticed that now that the COMPUTE_HEADER_DEPENDENCIES feature is
> turned on automatically for compilers that support it (see
> v1.7.8-rc0~142^2~1, 2011-08-18), there is no easy way to force it off.
> For example, setting COMPUTE_HEADER_DEPENDENCIES to the empty string
> in config.mak just tells the makefile to treat it as undefined and
> run a test command to see if the -MMD option is supported.
>
> Introduce a new NO_COMPUTE_HEADER_DEPENDENCIES variable that forces
> the feature off.
Eek. At least at the end user UI level, couldn't we do this as a tristate?
E.g. "YesPlease" (or anything that begins with Y if you are ambitious) to
explicitly enable, empty (or "auto") to autodetect, and anything else to
decline?
Even better, couldn't we either (1) rearrange .dep/ files somehow, so that
compiler difference does not matter, or (2) have dep_check to perform a
trial run to detect versions of compilers that produce the output that we
cannot use?
^ permalink raw reply
* Re: t/t1304: avoid -d option to setfacl (db82657)
From: Brandon Casey @ 2011-11-18 5:37 UTC (permalink / raw)
To: wsp; +Cc: git
In-Reply-To: <gitster/git/commit/db826571e4099066fe44233d95642591016c831b/729354@github.com>
[cc git@vger.kernel.org since that is the appropriate place for this discusion]
On Thu, Nov 17, 2011 at 8:59 PM, wsp
<reply+c-729354-3460aca0fa61e627f9d1a271cf70a99d5c1e7e4e-921167@reply.github.com>
wrote:
> Could this test case be reviewed again? It fails on FreeBSD where the appropriate way to specify default ACL's is with the "-d" option. The "d[efault]:" syntax is invalid on FreeBSD.
Well, I'm not sure there is a right answer here.
Linux accepts -d or "d[efault]"
Solaris only accepts "d[efault]"
FreeBSD only accepts -d
a quick search shows z/OS only accepts "d[efault]"
other?
I think most everything else rolls their own implementation into chmod
or chacl or something like that.
The abandoned POSIX draft does actually specify the FreeBSD behavior.
So I think it's kind of a toss-up. Which option we choose should
probably depend on whether we get more test coverage by using the
"d[efault]" notation or by using the -d option. That depends on
whether there are more Solaris users compiling git or whether there
are more FreeBSD users. I don't know the answer to that either. I
tend to think there are very few of either.
-Brandon
^ permalink raw reply
* test t/t1304-default-acl.sh
From: Scott Parrish @ 2011-11-18 5:10 UTC (permalink / raw)
To: Git Mailing List
Hi folks,
This test case fails on FreeBSD 8.2. Specifically, the use of the "d[efault]:" syntax in the ACL tag is invalid on FreeBSD. The manpage indicates that default ACL's should be set via the "-d" command line option to setfacl. See http://www.FreeBSD.org/cgi/man.cgi?query=setfacl&apropos=0&sektion=0&manpath=FreeBSD+8.2-RELEASE&arch=default&format=html for details.
Note that the test used "-d" at one time but appears to have been changed to use "d:" to accommodate Solaris where "-d" has the effect of deleting ACL entries. The change was made on this commit: https://github.com/gitster/git/commit/db826571e4099066fe44233d95642591016c831b
So I don't think it is as straight-forward as reverting to the use of "-d".
Thoughts?
Cheers,
-Scott
^ permalink raw reply
* Re: [PATCH] Makefile: add option to disable automatic dependency generation
From: Jonathan Nieder @ 2011-11-18 5:00 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Git Mailing List, Fredrik Kuivinen
In-Reply-To: <20111118045742.GA25145@elie.hsd1.il.comcast.net>
Jonathan Nieder wrote:
> - If COMPUTE_HEADER_DEPENDENCIES is empty and NO_COMPUTE_... is
> nonempty, the build uses gcc's on-the-fly dependency generation
> feature.
Erm. This should say:
"- If COMPUTE_HEADER_DEPENDENCIES is nonempty and NO_COMPUTE_... is
empty,"
Sorry for the noise.
^ permalink raw reply
* [PATCH] Makefile: add option to disable automatic dependency generation
From: Jonathan Nieder @ 2011-11-18 4:57 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Git Mailing List, Fredrik Kuivinen
In-Reply-To: <CACsJy8A44PFtYrm8NQU+48sVkOe8mjJyO9opO5-TwRtAd-TKsQ@mail.gmail.com>
Duy noticed that now that the COMPUTE_HEADER_DEPENDENCIES feature is
turned on automatically for compilers that support it (see
v1.7.8-rc0~142^2~1, 2011-08-18), there is no easy way to force it off.
For example, setting COMPUTE_HEADER_DEPENDENCIES to the empty string
in config.mak just tells the makefile to treat it as undefined and
run a test command to see if the -MMD option is supported.
Introduce a new NO_COMPUTE_HEADER_DEPENDENCIES variable that forces
the feature off. The new semantics:
- If NO_COMPUTE_HEADER_DEPENDENCIES is set to a nonempty string,
the -MMD option will not be used. The build relies on hard-coded
dependencies in the "ifndef USE_COMPUTED_HEADER_DEPENDENCIES"
section of the Makefile instead.
- If COMPUTE_HEADER_DEPENDENCIES is empty and NO_COMPUTE_... is
nonempty, the build uses gcc's on-the-fly dependency generation
feature.
- If neither is nonempty, the makefile runs a quick test command
to decide whether the compiler supports the -MMD option and
whether to enable this feature.
Inspired-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Nguyen Thai Ngoc Duy wrote:
> Hmm.. I guess it's my compiler's fault then. Next question, how can I
> disable this feature?
"make COMPUTE_HEADER_DEPENDENCIES=" works. But there's no way aside
from "override COMPUTE_HEADER_DEPENDENCIES=" to do that in config.mak.
And yuck.
By the way, I'm not convinced it's your compiler's fault. It might be
my compiler's fault. More reading to do...
Makefile | 15 ++++++++++++++-
1 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/Makefile b/Makefile
index ee34eab8..e4a658f6 100644
--- a/Makefile
+++ b/Makefile
@@ -250,6 +250,13 @@ all::
# DEFAULT_EDITOR='$GIT_FALLBACK_EDITOR',
# DEFAULT_EDITOR='"C:\Program Files\Vim\gvim.exe" --nofork'
#
+# Define COMPUTE_HEADER_DEPENDENCIES if your compiler supports the -MMD option
+# and you want to avoid rebuilding objects when an unrelated header file
+# changes.
+#
+# Define NO_COMPUTE_HEADER_DEPENDENCIES if you want to disable automatic
+# dependency generation even though your compiler is detected to support it.
+#
# Define CHECK_HEADER_DEPENDENCIES to check for problems in the hard-coded
# dependency rules.
#
@@ -1245,10 +1252,16 @@ endif
endif
ifdef CHECK_HEADER_DEPENDENCIES
+NO_COMPUTE_HEADER_DEPENDENCIES = YesPlease
+endif
+
+ifdef NO_COMPUTE_HEADER_DEPENDENCIES
COMPUTE_HEADER_DEPENDENCIES =
USE_COMPUTED_HEADER_DEPENDENCIES =
-else
+endif
+
ifndef COMPUTE_HEADER_DEPENDENCIES
+ifndef NO_COMPUTE_HEADER_DEPENDENCIES
dep_check = $(shell $(CC) $(ALL_CFLAGS) \
-c -MF /dev/null -MMD -MP -x c /dev/null -o /dev/null 2>&1; \
echo $$?)
--
1.7.8.rc2
^ permalink raw reply related
* Re: A flaw in dep generation with gcc -MMD?
From: Miles Bader @ 2011-11-18 4:49 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Jonathan Nieder, Git Mailing List
In-Reply-To: <CACsJy8A44PFtYrm8NQU+48sVkOe8mjJyO9opO5-TwRtAd-TKsQ@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>> Interesting. What compiler do you use?
>
> $ gcc --version
> gcc (Gentoo 4.4.4-r2 p1.2, pie-0.4.5) 4.4.4
FWIW, gcc 4.4.6 on debian does the correct thing too...
-Miles
--
Yo mama's so fat when she gets on an elevator it HAS to go down.
^ permalink raw reply
* Re: Git Gems
From: Ramkumar Ramachandra @ 2011-11-18 4:38 UTC (permalink / raw)
To: Hilco Wijbenga; +Cc: Git List
In-Reply-To: <CAE1pOi1gyshz_502NQvLNAByfwiYXW2fzA+EnGKz8tuFrCpkxg@mail.gmail.com>
Hi Hilco,
Hilco Wijbenga wrote:
> [CC: Git Users <git@vger.kernel.org>]
For the record, this is the official Git list: both "developers" and
"users" hang out here.
> [...]
> As an example, 'git rebase' didn't really seem useful until I
> understood (well, more or less) what it did. Until then, I just
> naively assumed that merge commits and non-linear history were
> something you simply had to live with. I'm guessing that, like me, a
> lot of people come to Git with quite a few assumptions and
> preconceived notions due to their exposure to other SCM tools. :-(
I'm not sure how a listing is going to help; nevertheless, here are a
few of the lesser-known features of the top of my head (in no
particular order): rerere, attributes, replace refs, filter-branch,
blame's -C and -M switches, log's -S switch, custom diff drivers,
bundle, submodule, stash, notes, and reflog.
[You can find more by digging through the sources]
Which brings us to an interesting aside: unlike many other SCMs which
have a definite and finite set of features, git is really just a
toolkit that grows everyday- various people use various subsets and
write up their own custom scripts to help them automate tasks. The
"git rev-list/ git rev-parse/ git cat-file" is an awesome trio to
start writing shell scripts with :)
Cheers.
-- Ram
^ permalink raw reply
* Re: A flaw in dep generation with gcc -MMD?
From: Nguyen Thai Ngoc Duy @ 2011-11-18 3:56 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git Mailing List
In-Reply-To: <20111118034142.GA25228@elie.hsd1.il.comcast.net>
On Fri, Nov 18, 2011 at 10:41 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Nguyen Thai Ngoc Duy wrote:
>
>> My builtin/.depend/add.o.d says
>>
>> add.o: .... cache.h ...
>
> Interesting. What compiler do you use?
$ gcc --version
gcc (Gentoo 4.4.4-r2 p1.2, pie-0.4.5) 4.4.4
> $ head -1 builtin/.depend/add.o.d
> builtin/add.o: builtin/add.c cache.h git-compat-util.h compat/bswap.h \
> $ gcc --version | head -1
> gcc (Debian 4.6.2-4) 4.6.2
Hmm.. I guess it's my compiler's fault then. Next question, how can I
disable this feature?
--
Duy
^ permalink raw reply
* Re: A flaw in dep generation with gcc -MMD?
From: Jonathan Nieder @ 2011-11-18 3:41 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Git Mailing List
In-Reply-To: <CACsJy8BZMDyf4MCiKxPJ5Z+XS+C-MC82SpMFyWgiXmb9xCnScw@mail.gmail.com>
Nguyen Thai Ngoc Duy wrote:
> My builtin/.depend/add.o.d says
>
> add.o: .... cache.h ...
Interesting. What compiler do you use?
Thanks for finding it,
Jonathan
$ head -1 builtin/.depend/add.o.d
builtin/add.o: builtin/add.c cache.h git-compat-util.h compat/bswap.h \
$ gcc --version | head -1
gcc (Debian 4.6.2-4) 4.6.2
^ permalink raw reply
* Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage
From: Nguyen Thai Ngoc Duy @ 2011-11-18 3:33 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Ramkumar Ramachandra, Junio C Hamano, Git List
In-Reply-To: <20111116075955.GB13706@elie.hsd1.il.comcast.net>
On Wed, Nov 16, 2011 at 01:59:55AM -0600, Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> > Junio C Hamano wrote:
>
> >> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436
> >
> > I noticed that sha1_to_hex() also operates like this. The
> > resolve_ref() function is really important, but using the same
> > technique for these tiny functions is probably an overkill
>
> I don't follow. Do you mean that not being confusing is overkill,
> because the function is small that no one will bother to look up the
> right semantics? Wait, that sentence didn't come out the way I
> wanted. ;-)
>
> Jokes aside, here's a rough series to do the git_path ->
> git_path_unsafe renaming. While writing it, I noticed a couple of
> bugs, hence the two patches before the last one. Patch 2 is the more
> interesting one.
Or perhaps we can use per-file buffer rings instead of a global one.
This means git_path() can only interfere another one in the same file,
making the interaction simpler and hopefully simple enough for reviewers
to catch 90% bugs, therefore safe enough to avoid the _unsafe suffix.
Adding static variable declaration in cache.h is ugly, but that could be
moved to a separate header file.
diff --git a/cache.h b/cache.h
index 2e6ad36..437bc3a 100644
--- a/cache.h
+++ b/cache.h
@@ -660,9 +660,13 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
extern char *git_pathdup(const char *fmt, ...)
__attribute__((format (printf, 1, 2)));
+#define git_path(...) git_path_1(pathname_array[3 & ++pathname_index], __VA_ARGS__)
+static char pathname_array[4][PATH_MAX];
+static int pathname_index;
+
/* Return a statically allocated filename matching the sha1 signature */
extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
-extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
+extern char *git_path_1(char *pathname, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
extern char *git_path_submodule(const char *path, const char *fmt, ...)
__attribute__((format (printf, 2, 3)));
diff --git a/path.c b/path.c
index b6f71d1..3c95db1 100644
--- a/path.c
+++ b/path.c
@@ -101,10 +101,9 @@ char *mkpath(const char *fmt, ...)
return cleanup_path(pathname);
}
-char *git_path(const char *fmt, ...)
+char *git_path_1(char *pathname, const char *fmt, ...)
{
const char *git_dir = get_git_dir();
- char *pathname = get_pathname();
va_list args;
unsigned len;
^ permalink raw reply related
* A flaw in dep generation with gcc -MMD?
From: Nguyen Thai Ngoc Duy @ 2011-11-18 2:24 UTC (permalink / raw)
To: Git Mailing List
Hi,
My builtin/.depend/add.o.d says
add.o: .... cache.h ...
Shouldn't it be "builtin/add.o: ... cache.h ..."? I tried to touch
cache.h and "make builtin/add.o". It did not remake builtin/add.o. If
I modify add.o.d by hand and remake, it works.
--
Duy
^ permalink raw reply
* Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()
From: Jeff King @ 2011-11-18 2:06 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Ramkumar Ramachandra, git, Junio C Hamano, Michael Haggerty,
Jonathan Nieder
In-Reply-To: <CACsJy8CB6VXjyC-M4C9qGm-n73Kuf1Q0SbH4Ync5Osts-uufQQ@mail.gmail.com>
On Fri, Nov 18, 2011 at 08:50:20AM +0700, Nguyen Thai Ngoc Duy wrote:
> > Unless you really need macro-like behavior, you're probably better off
> > using a variadic function and making it a static inline on platforms
> > which can do so.
>
> I need to save __FILE__ and __LINE__ of call site, inline functions
> probably don't help.
Yeah, you'd have to pass them in to the function. Which of course you
can't wrap with a macro, because the whole thing is variadic.
-Peff
^ permalink raw reply
* Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()
From: Nguyen Thai Ngoc Duy @ 2011-11-18 1:50 UTC (permalink / raw)
To: Jeff King
Cc: Ramkumar Ramachandra, git, Junio C Hamano, Michael Haggerty,
Jonathan Nieder
In-Reply-To: <20111118012715.GA7826@sigill.intra.peff.net>
On Fri, Nov 18, 2011 at 8:27 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Nov 18, 2011 at 08:12:27AM +0700, Nguyen Thai Ngoc Duy wrote:
>
>> > Older compilers will probably barf on the variable-argument macros.
>>
>> Anyway to detect if __VA_ARGS__ is supported at compile time? I guess
>> #ifdef __GNUC__ is the last resort.
>
> You can check "#if __STDC_VERSION__ >= 19901L", but that will of course
> only tell you whether you have C99; older gcc (and possibly other
> compilers) supported __VA_ARGS__ even before it was standardized.
>
> But more annoying is that there isn't a great fallback to __VA_ARGS__.
> If you can't use it, then every callsite has to have the same number of
> arguments. So it's not like you can localize the fallback code to just
> the definition.
>
> Unless you really need macro-like behavior, you're probably better off
> using a variadic function and making it a static inline on platforms
> which can do so.
I need to save __FILE__ and __LINE__ of call site, inline functions
probably don't help.
>> notes-merge.c introduces __VA_ARGS__ since v1.7.4 so we may want to do
>> something there too.
>
> I hadn't noticed. That definitely violates our usual rules about
> portability. That usage can easily be turned into an inline function.
> However, since nobody has complained in the past year, it makes me
> wonder if we are overly conservative (my guess is that people on crazy
> old compilers just don't keep up with git. Which maybe means they aren't
> worth worrying about. But who knows).
For the record, Sun C compiler 5.9 seems to support it.
--
Duy
^ permalink raw reply
* [PATCH] Remove useless code about diffcore_count_changes()
From: Zheng Liu @ 2011-11-18 1:43 UTC (permalink / raw)
To: git; +Cc: Zheng Liu
We don't need to check return value because it always returns zero in
diffcore_count_changes().
Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
---
diffcore-break.c | 7 ++-----
diffcore-rename.c | 7 ++-----
2 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/diffcore-break.c b/diffcore-break.c
index 44f8678..402bba1 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -68,11 +68,8 @@ static int should_break(struct diff_filespec *src,
if (max_size < MINIMUM_BREAK_SIZE)
return 0; /* we do not break too small filepair */
- if (diffcore_count_changes(src, dst,
- &src->cnt_data, &dst->cnt_data,
- 0,
- &src_copied, &literal_added))
- return 0;
+ diffcore_count_changes(src, dst, &src->cnt_data, &dst->cnt_data, 0,
+ &src_copied, &literal_added);
/* sanity */
if (src->size < src_copied)
diff --git a/diffcore-rename.c b/diffcore-rename.c
index f639601..969482f 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -182,11 +182,8 @@ static int estimate_similarity(struct diff_filespec *src,
delta_limit = (unsigned long)
(base_size * (MAX_SCORE-minimum_score) / MAX_SCORE);
- if (diffcore_count_changes(src, dst,
- &src->cnt_data, &dst->cnt_data,
- delta_limit,
- &src_copied, &literal_added))
- return 0;
+ diffcore_count_changes(src, dst, &src->cnt_data, &dst->cnt_data,
+ delta_limit, &src_copied, &literal_added);
/* How similar are they?
* what percentage of material in dst are from source?
--
1.7.5.4
^ permalink raw reply related
* [PATCH] Use macro define to replace magic character
From: Zheng Liu @ 2011-11-18 1:43 UTC (permalink / raw)
To: git; +Cc: Zheng Liu
We should use macro define rather than magic character to make more readable
Signed-off-by: Zheng Liu <wenqing.lz@taobao.com>
---
merge-recursive.c | 2 +-
tree-diff.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/merge-recursive.c b/merge-recursive.c
index cc664c3..96457bc 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -503,7 +503,7 @@ static struct string_list *get_renames(struct merge_options *o,
struct string_list_item *item;
struct rename *re;
struct diff_filepair *pair = diff_queued_diff.queue[i];
- if (pair->status != 'R') {
+ if (pair->status != DIFF_STATUS_RENAMED) {
diff_free_filepair(pair);
continue;
}
diff --git a/tree-diff.c b/tree-diff.c
index b3cc2e4..d52c440 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -225,7 +225,7 @@ static void try_to_follow_renames(struct tree_desc *t1, struct tree_desc *t2, co
* diff_queued_diff, we will also use that as the path in
* the future!
*/
- if ((p->status == 'R' || p->status == 'C') &&
+ if ((p->status == DIFF_STATUS_RENAMED || p->status == DIFF_STATUS_COPIED) &&
!strcmp(p->two->path, opt->pathspec.raw[0])) {
/* Switch the file-pairs around */
q->queue[i] = choice;
--
1.7.5.4
^ permalink raw reply related
* Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()
From: Jonathan Nieder @ 2011-11-18 1:36 UTC (permalink / raw)
To: Jeff King
Cc: Nguyen Thai Ngoc Duy, Ramkumar Ramachandra, git, Junio C Hamano,
Michael Haggerty
In-Reply-To: <20111118012715.GA7826@sigill.intra.peff.net>
Jeff King wrote:
> On Fri, Nov 18, 2011 at 08:12:27AM +0700, Nguyen Thai Ngoc Duy wrote:
>> notes-merge.c introduces __VA_ARGS__ since v1.7.4 so we may want to do
>> something there too.
>
> I hadn't noticed. That definitely violates our usual rules about
> portability. That usage can easily be turned into an inline function.
> However, since nobody has complained in the past year, it makes me
> wonder if we are overly conservative (my guess is that people on crazy
> old compilers just don't keep up with git. Which maybe means they aren't
> worth worrying about. But who knows).
I suspect we didn't notice because MSVC 2005 and later have some
(limited, as far as I can tell from web searches) support for
__VA_ARGS__.
^ permalink raw reply
* Re: [PATCH/RFC] introduce strbuf_addpath()
From: Nguyen Thai Ngoc Duy @ 2011-11-18 1:42 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Ramkumar Ramachandra, Junio C Hamano, Git List
In-Reply-To: <20111116215004.GA29872@elie.hsd1.il.comcast.net>
On Wed, Nov 16, 2011 at 03:50:04PM -0600, Jonathan Nieder wrote:
> strbuf_addpath() is like git_path_unsafe(), except instead of
> returning its own buffer, it appends its result to a buffer provided
> by the caller.
>
> Benefits:
>
> - Since it uses a caller-supplied buffer, unlike git_path_unsafe(),
> there is no risk that one call will clobber the result from
> another.
>
> - Unlike git_pathdup(), it does not need to waste time allocating
> memory in the middle of your tight loop over refs.
>
> - The size of the result is not limited to PATH_MAX.
>
> Caveat: the size of its result is not limited to PATH_MAX. Existing
> code might be relying on git_path*() to produce a result that is safe
> to copy to a PATH_MAX-sized buffer. Be careful.
>
> This patch introduces the strbuf_addpath() function and converts a few
> existing users of the strbuf_addstr(git_path(...)) idiom to
> demonstrate the API.
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Jonathan Nieder wrote:
>
> > I think if I ran the world, the fundamental operation would be
> > strbuf_addpath().
>
> Like this, maybe.
>
If I ran the world, I would change get_pathname() to return "struct
strbuf *" instead and change "char *git_path(..)" to "const char *git_path(...)".
Code paths that want to modify git_path() return value could
just use strbuf_addpath()
I did try to turn git_path to return const char *, the following patch
seems to make it build without any warnings
-- 8< --
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c6bc8eb..fd7c682 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1041,7 +1041,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
if (args.depth > 0) {
struct cache_time mtime;
struct strbuf sb = STRBUF_INIT;
- char *shallow = git_path("shallow");
+ const char *shallow = git_path("shallow");
int fd;
mtime.sec = st.st_mtime;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 91731b9..261f1a0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -367,7 +367,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
char note[1024];
const char *what, *kind;
struct ref *rm;
- char *url, *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
+ const char *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
+ char *url;
fp = fopen(filename, "a");
if (!fp)
@@ -647,7 +648,7 @@ static void check_not_current_branch(struct ref *ref_map)
static int truncate_fetch_head(void)
{
- char *filename = git_path("FETCH_HEAD");
+ const char *filename = git_path("FETCH_HEAD");
FILE *fp = fopen(filename, "w");
if (!fp)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0e0e17a..f9624d7 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -215,12 +215,12 @@ static void check_unreachable_object(struct object *obj)
printf("dangling %s %s\n", typename(obj->type),
sha1_to_hex(obj->sha1));
if (write_lost_and_found) {
- char *filename = git_path("lost-found/%s/%s",
+ const char *filename = git_path("lost-found/%s/%s",
obj->type == OBJ_COMMIT ? "commit" : "other",
sha1_to_hex(obj->sha1));
FILE *f;
- if (safe_create_leading_directories(filename)) {
+ if (safe_create_leading_directories_const(filename)) {
error("Could not create lost-found");
return;
}
diff --git a/builtin/remote.c b/builtin/remote.c
index 583eec9..0662d37 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -587,7 +587,7 @@ static int migrate_file(struct remote *remote)
{
struct strbuf buf = STRBUF_INIT;
int i;
- char *path = NULL;
+ const char *path = NULL;
strbuf_addf(&buf, "remote.%s.url", remote->name);
for (i = 0; i < remote->url_nr; i++)
diff --git a/fast-import.c b/fast-import.c
index 8d8ea3c..4293a9f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -403,7 +403,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);
static void write_crash_report(const char *err)
{
- char *loc = git_path("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
+ const char *loc = git_path("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
FILE *rpt = fopen(loc, "w");
struct branch *b;
unsigned long lu;
diff --git a/notes-merge.c b/notes-merge.c
index e33c2c9..8da2e0a 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -288,7 +288,7 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
"(%s exists).", git_path("NOTES_MERGE_*"));
}
- if (safe_create_leading_directories(git_path(
+ if (safe_create_leading_directories_const(git_path(
NOTES_MERGE_WORKTREE "/.test")))
die_errno("unable to create directory %s",
git_path(NOTES_MERGE_WORKTREE));
@@ -303,8 +303,8 @@ static void write_buf_to_worktree(const unsigned char *obj,
const char *buf, unsigned long size)
{
int fd;
- char *path = git_path(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
- if (safe_create_leading_directories(path))
+ const char *path = git_path(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
+ if (safe_create_leading_directories_const(path))
die_errno("unable to create directory for '%s'", path);
if (file_exists(path))
die("found existing file at '%s'", path);
diff --git a/refs.c b/refs.c
index 62d8a37..7469cf1 100644
--- a/refs.c
+++ b/refs.c
@@ -1189,7 +1189,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char *old_sha1, int flags, int *type_p)
{
- char *ref_file;
+ const char *ref_file;
const char *orig_ref = ref;
struct ref_lock *lock;
int last_errno = 0;
@@ -1250,7 +1250,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
lock->force_write = 1;
- if (safe_create_leading_directories(ref_file)) {
+ if (safe_create_leading_directories_const(ref_file)) {
last_errno = errno;
error("unable to create directory for %s", ref_file);
goto error_return;
@@ -1411,7 +1411,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
}
}
- if (log && safe_create_leading_directories(git_path("logs/%s", newref))) {
+ if (log && safe_create_leading_directories_const(git_path("logs/%s", newref))) {
error("unable to create directory for %s", newref);
goto rollback;
}
-- 8< --
^ permalink raw reply related
* Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()
From: Jonathan Nieder @ 2011-11-18 1:27 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Jeff King, Ramkumar Ramachandra, git, Junio C Hamano,
Michael Haggerty, Johan Herland
In-Reply-To: <CACsJy8A25SyLVKv8GwkYaHBJwU5tHqgdJK6L-upF9HWseFzCtQ@mail.gmail.com>
Nguyen Thai Ngoc Duy wrote:
> Anyway to detect if __VA_ARGS__ is supported at compile time? I guess
> #ifdef __GNUC__ is the last resort.
Why would one want to do that? Either your feature requires C99 (so the
build can error out if __VA_ARGS__ support is missing) or it doesn't.
> notes-merge.c introduces __VA_ARGS__ since v1.7.4 so we may want to do
> something there too.
Good catch. How about this patch? It seems I forgot to send it out
last May and it has been rotting in my local tree ever since.
-- >8 --
Subject: notes merge: eliminate OUTPUT macro
The macro is variadic, which breaks support for pre-C99 compilers,
and it hides an "if", which can make code hard to understand on
first reading if some arguments have side-effects.
The OUTPUT macro seems to have been inspired by the "output" function
from merge-recursive. But that function in merge-recursive exists to
indent output based on the level of recursion and there is no similar
justification for such a function in "notes merge".
Noticed with 'make CC="gcc -std=c89 -pedantic"':
notes-merge.c:24:22: warning: anonymous variadic macros were introduced in C99 [-Wvariadic-macros]
Encouraged-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
notes-merge.c | 104 +++++++++++++++++++++++++++++++++-----------------------
1 files changed, 61 insertions(+), 43 deletions(-)
diff --git a/notes-merge.c b/notes-merge.c
index baaf31f4..d0e5034d 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -21,14 +21,6 @@ void init_notes_merge_options(struct notes_merge_options *o)
o->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT;
}
-#define OUTPUT(o, v, ...) \
- do { \
- if ((o)->verbosity >= (v)) { \
- printf(__VA_ARGS__); \
- puts(""); \
- } \
- } while (0)
-
static int path_to_sha1(const char *path, unsigned char *sha1)
{
char hex_sha1[40];
@@ -392,21 +384,26 @@ static int merge_one_change_manual(struct notes_merge_options *o,
strbuf_addf(&(o->commit_msg), "\t%s\n", sha1_to_hex(p->obj));
- OUTPUT(o, 2, "Auto-merging notes for %s", sha1_to_hex(p->obj));
+ if (o->verbosity >= 2)
+ printf("Auto-merging notes for %s\n", sha1_to_hex(p->obj));
check_notes_merge_worktree(o);
if (is_null_sha1(p->local)) {
/* D/F conflict, checkout p->remote */
assert(!is_null_sha1(p->remote));
- OUTPUT(o, 1, "CONFLICT (delete/modify): Notes for object %s "
- "deleted in %s and modified in %s. Version from %s "
- "left in tree.", sha1_to_hex(p->obj), lref, rref, rref);
+ if (o->verbosity >= 1)
+ printf("CONFLICT (delete/modify): Notes for object %s "
+ "deleted in %s and modified in %s. Version from %s "
+ "left in tree.\n",
+ sha1_to_hex(p->obj), lref, rref, rref);
write_note_to_worktree(p->obj, p->remote);
} else if (is_null_sha1(p->remote)) {
/* D/F conflict, checkout p->local */
assert(!is_null_sha1(p->local));
- OUTPUT(o, 1, "CONFLICT (delete/modify): Notes for object %s "
- "deleted in %s and modified in %s. Version from %s "
- "left in tree.", sha1_to_hex(p->obj), rref, lref, lref);
+ if (o->verbosity >= 1)
+ printf("CONFLICT (delete/modify): Notes for object %s "
+ "deleted in %s and modified in %s. Version from %s "
+ "left in tree.\n",
+ sha1_to_hex(p->obj), rref, lref, lref);
write_note_to_worktree(p->obj, p->local);
} else {
/* "regular" conflict, checkout result of ll_merge() */
@@ -415,8 +412,9 @@ static int merge_one_change_manual(struct notes_merge_options *o,
reason = "add/add";
assert(!is_null_sha1(p->local));
assert(!is_null_sha1(p->remote));
- OUTPUT(o, 1, "CONFLICT (%s): Merge conflict in notes for "
- "object %s", reason, sha1_to_hex(p->obj));
+ if (o->verbosity >= 1)
+ printf("CONFLICT (%s): Merge conflict in notes for "
+ "object %s\n", reason, sha1_to_hex(p->obj));
ll_merge_in_worktree(o, p);
}
@@ -438,24 +436,30 @@ static int merge_one_change(struct notes_merge_options *o,
case NOTES_MERGE_RESOLVE_MANUAL:
return merge_one_change_manual(o, p, t);
case NOTES_MERGE_RESOLVE_OURS:
- OUTPUT(o, 2, "Using local notes for %s", sha1_to_hex(p->obj));
+ if (o->verbosity >= 2)
+ printf("Using local notes for %s\n",
+ sha1_to_hex(p->obj));
/* nothing to do */
return 0;
case NOTES_MERGE_RESOLVE_THEIRS:
- OUTPUT(o, 2, "Using remote notes for %s", sha1_to_hex(p->obj));
+ if (o->verbosity >= 2)
+ printf("Using remote notes for %s\n",
+ sha1_to_hex(p->obj));
if (add_note(t, p->obj, p->remote, combine_notes_overwrite))
die("BUG: combine_notes_overwrite failed");
return 0;
case NOTES_MERGE_RESOLVE_UNION:
- OUTPUT(o, 2, "Concatenating local and remote notes for %s",
- sha1_to_hex(p->obj));
+ if (o->verbosity >= 2)
+ printf("Concatenating local and remote notes for %s\n",
+ sha1_to_hex(p->obj));
if (add_note(t, p->obj, p->remote, combine_notes_concatenate))
die("failed to concatenate notes "
"(combine_notes_concatenate)");
return 0;
case NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ:
- OUTPUT(o, 2, "Concatenating unique lines in local and remote "
- "notes for %s", sha1_to_hex(p->obj));
+ if (o->verbosity >= 2)
+ printf("Concatenating unique lines in local and remote "
+ "notes for %s\n", sha1_to_hex(p->obj));
if (add_note(t, p->obj, p->remote, combine_notes_cat_sort_uniq))
die("failed to concatenate notes "
"(combine_notes_cat_sort_uniq)");
@@ -518,8 +522,9 @@ static int merge_from_diffs(struct notes_merge_options *o,
conflicts = merge_changes(o, changes, &num_changes, t);
free(changes);
- OUTPUT(o, 4, "Merge result: %i unmerged notes and a %s notes tree",
- conflicts, t->dirty ? "dirty" : "clean");
+ if (o->verbosity >= 4)
+ printf("Merge result: %i unmerged notes and a %s notes tree\n",
+ conflicts, t->dirty ? "dirty" : "clean");
return conflicts ? -1 : 1;
}
@@ -616,33 +621,40 @@ int notes_merge(struct notes_merge_options *o,
if (!bases) {
base_sha1 = null_sha1;
base_tree_sha1 = EMPTY_TREE_SHA1_BIN;
- OUTPUT(o, 4, "No merge base found; doing history-less merge");
+ if (o->verbosity >= 4)
+ printf("No merge base found; doing history-less merge\n");
} else if (!bases->next) {
base_sha1 = bases->item->object.sha1;
base_tree_sha1 = bases->item->tree->object.sha1;
- OUTPUT(o, 4, "One merge base found (%.7s)",
- sha1_to_hex(base_sha1));
+ if (o->verbosity >= 4)
+ printf("One merge base found (%.7s)\n",
+ sha1_to_hex(base_sha1));
} else {
/* TODO: How to handle multiple merge-bases? */
base_sha1 = bases->item->object.sha1;
base_tree_sha1 = bases->item->tree->object.sha1;
- OUTPUT(o, 3, "Multiple merge bases found. Using the first "
- "(%.7s)", sha1_to_hex(base_sha1));
+ if (o->verbosity >= 3)
+ printf("Multiple merge bases found. Using the first "
+ "(%.7s)\n", sha1_to_hex(base_sha1));
}
- OUTPUT(o, 4, "Merging remote commit %.7s into local commit %.7s with "
- "merge-base %.7s", sha1_to_hex(remote->object.sha1),
- sha1_to_hex(local->object.sha1), sha1_to_hex(base_sha1));
+ if (o->verbosity >= 4)
+ printf("Merging remote commit %.7s into local commit %.7s with "
+ "merge-base %.7s\n", sha1_to_hex(remote->object.sha1),
+ sha1_to_hex(local->object.sha1),
+ sha1_to_hex(base_sha1));
if (!hashcmp(remote->object.sha1, base_sha1)) {
/* Already merged; result == local commit */
- OUTPUT(o, 2, "Already up-to-date!");
+ if (o->verbosity >= 2)
+ printf("Already up-to-date!\n");
hashcpy(result_sha1, local->object.sha1);
goto found_result;
}
if (!hashcmp(local->object.sha1, base_sha1)) {
/* Fast-forward; result == remote commit */
- OUTPUT(o, 2, "Fast-forward");
+ if (o->verbosity >= 2)
+ printf("Fast-forward\n");
hashcpy(result_sha1, remote->object.sha1);
goto found_result;
}
@@ -684,8 +696,9 @@ int notes_merge_commit(struct notes_merge_options *o,
int path_len = strlen(path), i;
const char *msg = strstr(partial_commit->buffer, "\n\n");
- OUTPUT(o, 3, "Committing notes in notes merge worktree at %.*s",
- path_len - 1, path);
+ if (o->verbosity >= 3)
+ printf("Committing notes in notes merge worktree at %.*s\n",
+ path_len - 1, path);
if (!msg || msg[2] == '\0')
die("partial notes commit has empty message");
@@ -700,7 +713,9 @@ int notes_merge_commit(struct notes_merge_options *o,
unsigned char obj_sha1[20], blob_sha1[20];
if (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {
- OUTPUT(o, 3, "Skipping non-SHA1 entry '%s'", ent->name);
+ if (o->verbosity >= 3)
+ printf("Skipping non-SHA1 entry '%s'\n",
+ ent->name);
continue;
}
@@ -712,14 +727,16 @@ int notes_merge_commit(struct notes_merge_options *o,
if (add_note(partial_tree, obj_sha1, blob_sha1, NULL))
die("Failed to add resolved note '%s' to notes tree",
ent->name);
- OUTPUT(o, 4, "Added resolved note for object %s: %s",
- sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
+ if (o->verbosity >= 4)
+ printf("Added resolved note for object %s: %s\n",
+ sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));
}
create_notes_commit(partial_tree, partial_commit->parents, msg,
result_sha1);
- OUTPUT(o, 4, "Finalized notes merge commit: %s",
- sha1_to_hex(result_sha1));
+ if (o->verbosity >= 4)
+ printf("Finalized notes merge commit: %s\n",
+ sha1_to_hex(result_sha1));
free(path);
return 0;
}
@@ -731,7 +748,8 @@ int notes_merge_abort(struct notes_merge_options *o)
int ret;
strbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));
- OUTPUT(o, 3, "Removing notes merge worktree at %s", buf.buf);
+ if (o->verbosity >= 3)
+ printf("Removing notes merge worktree at %s\n", buf.buf);
ret = remove_dir_recursively(&buf, 0);
strbuf_release(&buf);
return ret;
--
1.7.8.rc2
^ 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