What's cooking in git.git (Jun 2012, #02; Wed, 6)

What's cooking in git.git (Jun 2012, #02; Wed, 6)

[Junio C Hamano]

What's cooking in git.git (Jun 2012, #02; Wed, 6)
--------------------------------------------------

Here are the topics that have been cooking.  Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.

I'm planning to tag 1.7.11-rc2 tomorrow or Friday (we have slipped
by one week). Please concentrate on testing, finding and fixing
regressions introduced after 1.7.10 until 1.7.11 final.

You can find the changes described here in the integration branches of the
repositories listed at

    http://git-blame.blogspot.com/p/git-public-repositories.html

--------------------------------------------------
[New Topics]

* jc/bundle-complete-notice (2012-06-04) 1 commit
  (merged to 'next' on 2012-06-05 at ee25a35)
 + tweak "bundle verify" of a complete history

Running "git bundle verify" on a bundle that records a complete
history said "it requires these 0 commits".

Not urgent; probably early post 1.7.11.

* jc/svn-auth-providers-unusable-at-1.6.12 (2012-06-04) 1 commit
  (merged to 'next' on 2012-06-04 at c4ffe24)
 + git-svn: platform auth providers are working only on 1.6.15 or newer

Regression fix for people with libsvn between 1.6.12 and 1.6.15 on
which we tried to use non-working platform auth providers.

Will merge to master.

* lk/more-helpful-status-hints (2012-06-05) 4 commits
 - status: better advices when splitting a commit (during rebase -i)
 - status: don't suggest "git rm" or "git add" if not appropriate
 - t7512-status-help.sh: better advices for git status
 - wt-status.*: better advices for git status added

Almost there, modulo minor details pointed out by Matthieu.

* rr/maint-t3510-cascade-fix (2012-06-04) 1 commit
  (merged to 'next' on 2012-06-04 at 4c3368b)
 + t3510 (cherry-pick-sequence): add missing '&&'

Will merge to master.

* jc/merge-annotated-tag (2012-06-05) 2 commits
 - merge: allow fast-forwarding to an annotated but unsigned tag
 - merge: separte the logic to check for a signed tag

"git merge anno" created a merge commit even when anno is an
unsigned annotated tag that points at a commit that can be fast
forwarded to; this came from a laziness of the implementation of
merging of signed tags in 1.7.9, and can be worked around by saying
"git merge --ff-only anno", so it is not really necessariy, but is
here for completeness.

Not urgent.

* jk/no-more-pre-exec-callback (2012-06-05) 1 commit
 - pager: drop "wait for output to run less" hack

On hold for 6 months until ancient "less" goes extinct.

* jk/maint-t1304-setfacl (2012-06-06) 1 commit
 - t1304: improve setfacl prerequisite setup

Works around a false test failure caused by a bug in ecryptofs.

Not urgent; probably early post 1.7.11.

* lk/rebase-i-x (2012-06-06) 1 commit
 - rebase [-i --exec | -ix] <cmd>...

Adds -x <cmd> to "rebase -i" to insert "exec <cmd>" after each
commit in the resulting history.

Not urgent; probably early post 1.7.11.

* vr/help-per-platform (2012-06-06) 1 commit
 - help: use HTML as the default help format on Windows

* jc/fmt-merge-msg-people (2012-06-06) 1 commit
 - fmt-merge-msg: make attribution into comment lines

--------------------------------------------------
[Stalled]

* nd/exclude-workaround-top-heavy (2012-05-29) 2 commits
 - exclude: do strcmp as much as possible before fnmatch
 - Unindent excluded_from_list()

Attempts to optimize matching with an exclude pattern with a deep
directory hierarchy by taking the part that specifies leading path
without wildcard literally.

The code to check for wildcard needs to be redone.

* nl/http-proxy-more (2012-05-11) 2 commits
 - http: rename HTTP_REAUTH to HTTP_AUTH_RETRY
 - http: Avoid limit of retrying request only twice

I queued only the later two patches from this series, even though they do
not make much sense without the first one that seems to need a bit more
work, so that we won't forget.

* jk/no-op-push-message (2012-05-30) 1 commit
 - improve no-op push output

Rewords the status message of "git push" that pushed only one ref
differently from "Everything up-to-date", to give a bit more help to
people who get the message when their current branch is not pushed.

* fc/git-prompt-script (2012-05-22) 5 commits
 - completion: split __git_ps1 into a separate script
 - completion: remove executable mode
 - Merge branch 'fc/git-complete-helper' into fc/git-prompt-script
 - tests: add tests for the bash prompt functions in the completion script
 - tests: move code to run tests under bash into a helper library
 (this branch is tangled with sg/bash-prompt.)

The last remaining sticking point is what to do with the duplicated shell
function.

* sg/bash-prompt (2012-05-09) 4 commits
 - completion: respect $GIT_DIR
 - completion: use __gitdir() in _git_log()
 - tests: add tests for the bash prompt functions in the completion script
 - tests: move code to run tests under bash into a helper library
 (this branch is tangled with fc/git-prompt-script.)

This is only the "correction" bits taken from the beginning of a larger
series that is to be rerolled.  The first two are tangled with Felipe's
topic so a reroll, if comes, should build on top of them.

* jc/apply-3way (2012-06-06) 12 commits
 - apply: fix an incomplete message
 - apply: refactor "previous patch" logic
 - apply: a bit more comments on PATH_TO_BE_DELETED
 - apply: document --3way option
 - apply: allow rerere() upon --3way results
 - apply: register conflicted stages to the index
 - apply: plug the three-way merge logic in
 - apply: fall back on three-way merge
 - apply: accept -3/--3way command line option
 - apply: split load_preimage() helper function out
 - apply: refactor read_file_or_gitlink()
 - apply: clear_image() clears things a bit more

"git apply" learns to wiggle the base version and perform three-way merge
when a patch does not exactly apply to the version you have.

It turns out that it is somewhat unpleasant to handle add/add conflicts in
this code, but it seems necessary if we want to use "apply -3" to replace
the use of "apply --build-fake-ancestor" followed by the slow "merge" in
"am -3".

* jc/maint-push-refs-all (2012-05-04) 2 commits
 - get_fetch_map(): tighten checks on dest refs
 - fetch/push: allow refs/*:refs/*

Allows pushing and fetching refs/stash.
Not ready.
There still seem to be other bugs hiding (e.g. try pushing twice).

* jc/run-hook-env-1 (2012-03-11) 1 commit
 - run_hook(): enhance the interface to pass arbitrary environment

Not urgent.

Updates run_hook() API to be much less specific to "commit".  It would
only be useful if people start doing more interesting things with hooks.

* jc/split-blob (2012-04-03) 6 commits
 - chunked-object: streaming checkout
 - chunked-object: fallback checkout codepaths
 - bulk-checkin: support chunked-object encoding
 - bulk-checkin: allow the same data to be multiply hashed
 - new representation types in the packstream
 - packfile: use varint functions

Not ready.

I finished the streaming checkout codepath, but as explained in
127b177 (bulk-checkin: support chunked-object encoding, 2011-11-30),
these are still early steps of a long and painful journey. At least
pack-objects and fsck need to learn the new encoding for the series
to be usable locally, and then index-pack/unpack-objects needs to
learn it to be used remotely.

Given that I heard a lot of noise that people want large files, and
that I was asked by somebody at GitTogether'11 privately for an
advice on how to pay developers (not me) to help adding necessary
support, I am somewhat dissapointed that the original patch series
that was sent almost two months ago still remains here without much
comments and updates from the developer community. I even made the
interface to the logic that decides where to split chunks easily
replaceable, and I deliberately made the logic in the original patch
extremely stupid to entice others, especially the "bup" fanboys, to
come up with a better logic, thinking that giving people an easy
target to shoot for, they may be encouraged to help out. The plan is
not working :-(.

--------------------------------------------------
[Cooking]

* jc/ls-files-i-dir (2012-06-05) 6 commits
 - dir.c: make excluded() file scope static
 - unpack-trees.c: use path_excluded() in check_ok_to_remove()
 - builtin/add.c: use path_excluded()
 - path_excluded(): update API to less cache-entry centric
  (merged to 'next' on 2012-06-05 at 8f35db3)
 + ls-files -i: micro-optimize path_excluded()
 + ls-files -i: pay attention to exclusion of leading paths

"git ls-files --exclude=t -i" did not consider anything under t/
as excluded, as it did not pay attention to exclusion of leading
paths while walking the index.  Other two users of excluded() are
also updated.

Not urgent; probably early post 1.7.11.

* jc/request-pull-match-tagname (2012-06-01) 1 commit
  (merged to 'next' on 2012-06-05 at f0dc420)
 + request-pull: really favor a matching tag

"git request-pull $url dev" when the tip of "dev" branch was tagged
with "ext4-for-linus" used the contents from the tag in the output
but still asked the "dev" branch to be pulled, not the tag.

Not urgent; probably early post 1.7.11.

* db/vcs-svn (2012-06-01) 6 commits
 - vcs-svn: drop no-op reset methods
 - vcs-svn: fix signedness warnings
 - vcs-svn: prefer strstr over memmem
 - vcs-svn: prefer constcmp to prefixcmp
 - vcs-svn: simplify cleanup in apply_one_window()
 - vcs-svn: fix clang-analyzer error

I do not know the doneness of this series that came out of the
blue.  Are people involved in vcs-svn happy with this series?

* jk/version-string (2012-06-03) 3 commits
  (merged to 'next' on 2012-06-05 at b6f7266)
 + http: get default user-agent from git_user_agent
 + version: add git_user_agent function
 + move git_version_string into version.c

I am somewhat tempted to merge this to master before 1.7.11 final.
Thoughts?

* mm/api-credentials-doc (2012-06-04) 4 commits
  (merged to 'next' on 2012-06-05 at 8e14814)
 + api-credentials.txt: add "see also" section
 + api-credentials.txt: mention credential.helper explicitly
 + api-credentials.txt: show the big picture first
 + doc: fix xref link from api docs to manual pages

Will merge to master.

* nd/stream-pack-objects (2012-05-29) 1 commit
 - pack-objects: use streaming interface for reading large loose blobs

Not urgent; probably early post 1.7.11.

* jk/clone-local (2012-05-30) 2 commits
  (merged to 'next' on 2012-06-05 at b819eb5)
 + clone: allow --no-local to turn off local optimizations
 + docs/clone: mention that --local may be ignored

"git clone --local $path" started its life as an experiment to
optionally use link/copy when cloning a repository on the disk, but
we didn't deprecate it after we made the option a no-op to always
use the optimization.

Not urgent; probably early post 1.7.11.

* jk/no-more-asciidoc7 (2012-05-30) 2 commits
  (merged to 'next' on 2012-06-05 at 3f117df)
 + docs: drop antique comment from Makefile
 + docs: drop asciidoc7compatible flag

Not urgent; probably early post 1.7.11.

* cr/persistent-https (2012-05-30) 1 commit
  (merged to 'next' on 2012-06-01 at c647464)
 + Add persistent-https to contrib

A remote helper that acts as a proxy that caches ssl session for the
https:// transport is added to the contrib/ area.

Will merge to master.

* nd/stream-index-pack (2012-05-24) 4 commits
 - index-pack: use streaming interface for collision test on large blobs
 - index-pack: factor out unpack core from get_data_from_pack
 - index-pack: use streaming interface on large blobs (most of the time)
 - index-pack: hash non-delta objects while reading from stream

Use streaming API to read from the object store to avoid having to hold
a large blob object in-core while running index-pack.

Not urgent; probably early post 1.7.11.

* js/submodule-relative (2012-06-06) 4 commits
 - submodule: fix handling of superproject origin URLs like foo, ./foo and ./foo/bar
 - submodule: fix sync handling of some relative superproject origin URLs
 - submodule: document failure to handle relative superproject origin URLs
 - submodule: additional regression tests for relative URLs

Makes "git submodule" deal with nested submodule structure where a
module is contained within a module whose origin is specified as a
relative URL to its superproject's origin.

Not urgent; probably early post 1.7.11.

* mm/push-default-switch-warning (2012-06-06) 1 commit
 - push: start warning upcoming default change for push.default

Will merge to next after 1.7.11.

Hopwefully we can have a solidly tested series early in 1.7.12 or
1.7.13 at the latest.

--------------------------------------------------
[Discarded]

* cb/daemon-test-race-fix (2012-04-27) 2 commits
  (merged to 'next' on 2012-04-27 at 84bbcf8)
 + Revert "git-daemon wrapper to wait until daemon is ready"
  (merged to 'next' on 2012-04-24 at d5c30be)
 + git-daemon wrapper to wait until daemon is ready

Reverted from 'next' to replace it with js/daemon-test-race-fix.

Re: What's cooking in git.git (Jun 2012, #02; Wed, 6)

[David Michael Barr]

On Thu, Jun 7, 2012 at 7:55 AM, Junio C Hamano <gitster@pobox.com> wrote:
> * db/vcs-svn (2012-06-01) 6 commits
>  - vcs-svn: drop no-op reset methods
>  - vcs-svn: fix signedness warnings
>  - vcs-svn: prefer strstr over memmem
>  - vcs-svn: prefer constcmp to prefixcmp
>  - vcs-svn: simplify cleanup in apply_one_window()
>  - vcs-svn: fix clang-analyzer error
>
> I do not know the doneness of this series that came out of the
> blue.  Are people involved in vcs-svn happy with this series?

At the end of this series, vcs-svn is static-analyzer clean when
merged into svn-dump-fast-export.
I believe Jonathan's comments have been addressed.

Regarding minimising dependencies, it remains a long-term goal to make
vcs-svn available to other DVCS implementations.

--
David Barr

Re: What's cooking in git.git (Jun 2012, #02; Wed, 6)

[Jonathan Nieder]

Hi,

David Michael Barr wrote:
> On Thu, Jun 7, 2012 at 7:55 AM, Junio C Hamano <gitster@pobox.com> wrote:

>> * db/vcs-svn (2012-06-01) 6 commits
>>  - vcs-svn: drop no-op reset methods
>>  - vcs-svn: fix signedness warnings
>>  - vcs-svn: prefer strstr over memmem
>>  - vcs-svn: prefer constcmp to prefixcmp
>>  - vcs-svn: simplify cleanup in apply_one_window()
>>  - vcs-svn: fix clang-analyzer error
>>
>> I do not know the doneness of this series that came out of the
>> blue.  Are people involved in vcs-svn happy with this series?
>
> At the end of this series, vcs-svn is static-analyzer clean when
> merged into svn-dump-fast-export.

Thanks for the cc.  I generally like the code and generally dislike
the descriptions which amount to "appease the static analyzer" and
don't reflect the thought you actually put into the patches, which
sets a bad precedent.  I was planning to munge the descriptions, ask
you to look it over, and then ask Junio to pull.

The signedness patch still leaves me worried: for example, where len
is of type (size_t) and delta_len of type (off_t *),

	-       if (len > *delta_len ||
	+       if ((off_t) len > *delta_len ||

changes the meaning for the worse if len is very large on a system
(think: 64-bit PC) where size_t and off_t have the same width.  It's
exploitable. :(

But I like the spirit of the patches.

The "drop no-op reset methods" patch is unrisky and could go in
directly.

Jonathan

Re: What's cooking in git.git (Jun 2012, #02; Wed, 6)

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> At the end of this series, vcs-svn is static-analyzer clean when
>> merged into svn-dump-fast-export.
>
> Thanks for the cc.  I generally like the code and generally dislike
> the descriptions which amount to "appease the static analyzer" and
> don't reflect the thought you actually put into the patches, which
> sets a bad precedent.  I was planning to munge the descriptions, ask
> you to look it over, and then ask Junio to pull.

That sounds like a good way to go, then.

Thanks.

[GIT PULL] vcs-svn housekeeping

[Jonathan Nieder]

Hi Junio,

The following changes since commit 58ebd9865d2bb9d42842fbac5a1c4eae49e92859:

  vcs-svn/svndiff.c: squelch false "unused" warning from gcc (2012-01-27 11:58:56 -0800)

are available at:

  git://repo.or.cz/git/jrn.git svn-fe

The first three commits duplicate changes that are already in master
but were committed independently on the svn-fe branch last February.
The rest are David's db/vcs-svn series which aims to address various
nits noticed when merging the code back into svn-dump-fast-export:
unnecessary use of git-specific functions (prefixcmp, memmem) and
warnings reported by clang.

Some of the patches had to change a little since v2 of db/vcs-svn, so
I'll be replying with a copy of the patches for reference.

David has looked the branch over and acked and tested it.

Thoughts welcome, as usual.  I think these are ready for pulling into
master.  Sorry to be so slow at this.

David Barr (7):
      vcs-svn: drop no-op reset methods
      vcs-svn: avoid self-assignment in dummy initialization of pre_off
      vcs-svn: simplify cleanup in apply_one_window
      vcs-svn: use constcmp instead of prefixcmp
      vcs-svn: use strstr instead of memmem
      vcs-svn: suppress signed/unsigned comparison warnings
      vcs-svn: suppress a signed/unsigned comparison warning

Jonathan Nieder (4):
      vcs-svn: allow import of > 4GiB files
      vcs-svn: suppress -Wtype-limits warning
      vcs-svn: suppress a signed/unsigned comparison warning
      vcs-svn: allow 64-bit Prop-Content-Length

Ramsay Allan Jones (1):
      vcs-svn: rename check_overflow and its arguments for clarity

 test-line-buffer.c       |    1 -
 test-svn-fe.c            |    2 --
 vcs-svn/fast_export.c    |   26 ++++++++++++--------------
 vcs-svn/fast_export.h    |    5 ++---
 vcs-svn/line_buffer.c    |    4 ----
 vcs-svn/line_buffer.h    |    1 -
 vcs-svn/sliding_window.c |   16 ++++++++--------
 vcs-svn/svndiff.c        |   15 ++++++++-------
 vcs-svn/svndump.c        |   46 ++++++++++++++++++++++++++++------------------
 9 files changed, 58 insertions(+), 58 deletions(-)

[PATCH 1/9] vcs-svn: drop no-op reset methods

[Jonathan Nieder]

From: David Barr <davidbarr@google.com>
Date: Fri, 1 Jun 2012 00:41:30 +1000

Since v1.7.5~42^2~6 (vcs-svn: remove buffer_read_string)
buffer_reset() does nothing thus fast_export_reset() also.

Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Incorporates fixes from $gmane/198955.  No other changes.

 test-line-buffer.c    |    1 -
 test-svn-fe.c         |    2 --
 vcs-svn/fast_export.c |    5 -----
 vcs-svn/fast_export.h |    1 -
 vcs-svn/line_buffer.c |    4 ----
 vcs-svn/line_buffer.h |    1 -
 vcs-svn/svndump.c     |    2 --
 7 files changed, 16 deletions(-)

diff --git a/test-line-buffer.c b/test-line-buffer.c
index 7ec9b13c..ef1d7bae 100644
--- a/test-line-buffer.c
+++ b/test-line-buffer.c
@@ -87,6 +87,5 @@ int main(int argc, char *argv[])
 		die("input error");
 	if (ferror(stdout))
 		die("output error");
-	buffer_reset(&stdin_buf);
 	return 0;
 }
diff --git a/test-svn-fe.c b/test-svn-fe.c
index 332a5f71..83633a21 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -31,9 +31,7 @@ static int apply_delta(int argc, char *argv[])
 		die_errno("cannot close preimage");
 	if (buffer_deinit(&delta))
 		die_errno("cannot close delta");
-	buffer_reset(&preimage);
 	strbuf_release(&preimage_view.buf);
-	buffer_reset(&delta);
 	return 0;
 }
 
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index b823b851..b4be91cc 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -42,11 +42,6 @@ void fast_export_deinit(void)
 		die_errno("error closing fast-import feedback stream");
 }
 
-void fast_export_reset(void)
-{
-	buffer_reset(&report_buffer);
-}
-
 void fast_export_delete(const char *path)
 {
 	putchar('D');
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index aa629f54..8823aca1 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -6,7 +6,6 @@ struct line_buffer;
 
 void fast_export_init(int fd);
 void fast_export_deinit(void);
-void fast_export_reset(void);
 
 void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 01fcb842..57cc1cec 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -124,7 +124,3 @@ off_t buffer_skip_bytes(struct line_buffer *buf, off_t nbytes)
 	}
 	return done;
 }
-
-void buffer_reset(struct line_buffer *buf)
-{
-}
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 8901f214..ee23b4f4 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -14,7 +14,6 @@ struct line_buffer {
 int buffer_init(struct line_buffer *buf, const char *filename);
 int buffer_fdinit(struct line_buffer *buf, int fd);
 int buffer_deinit(struct line_buffer *buf);
-void buffer_reset(struct line_buffer *buf);
 
 int buffer_tmpfile_init(struct line_buffer *buf);
 FILE *buffer_tmpfile_rewind(struct line_buffer *buf);	/* prepare to write. */
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 644fdc71..f6c0d4c8 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -499,8 +499,6 @@ void svndump_deinit(void)
 
 void svndump_reset(void)
 {
-	fast_export_reset();
-	buffer_reset(&input);
 	strbuf_release(&dump_ctx.uuid);
 	strbuf_release(&dump_ctx.url);
 	strbuf_release(&rev_ctx.log);
-- 
1.7.10.4

[PATCH 2/9] vcs-svn: avoid self-assignment in dummy initialization of pre_off

[Jonathan Nieder]

From: David Barr <davidbarr@google.com>
Date: Fri, 1 Jun 2012 00:41:25 +1000

Without this change, clang complains:

 vcs-svn/svndiff.c:298:3: warning: Assigned value is garbage or undefined
                 off_t pre_off = pre_off; /* stupid GCC... */
                 ^               ~~~~~~~

This code uses an old and common idiom for suppressing an
"uninitialized variable" warning, and clang is wrong to warn about it.
The idiom tells the compiler to leave the variable uninitialized,
which saves a few bytes of code size, and, more importantly, allows
valgrind to check at runtime that the variable is properly initialized
by the time it is used.

But MSVC and clang do not know that idiom, so let's avoid it in
vcs-svn/ code.

Initialize pre_off to -1, a recognizably meaningless value, to allow
future code changes that cause pre_off to be used before it is
initialized to be caught early.

Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Changes since v2:

 - new commit message inspired by the discussion at [2]
 - initialize pre_off to -1 instead of 0

[1] http://thread.gmane.org/gmane.comp.version-control.git/198909/focus=198910
[2] http://thread.gmane.org/gmane.comp.version-control.git/169098/focus=169128

 vcs-svn/svndiff.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index 1647c1a7..c89d9623 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -295,7 +295,7 @@ int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
 	if (read_magic(delta, &delta_len))
 		return -1;
 	while (delta_len) {	/* For each window: */
-		off_t pre_off = pre_off; /* stupid GCC... */
+		off_t pre_off = -1;
 		size_t pre_len;
 
 		if (read_offset(delta, &pre_off, &delta_len) ||
-- 
1.7.10.4

[PATCH 3/9] vcs-svn: simplify cleanup in apply_one_window

[Jonathan Nieder]

From: David Barr <davidbarr@google.com>
Date: Fri, 1 Jun 2012 00:41:26 +1000

Currently the cleanup code looks like this:

	free resources
	return 0;
 error_out:
	free resources
	return -1;

Avoid duplicating the "free resources" part by keeping the return
value in a variable and sharing code between the success and
exceptional case:

	ret = 0;
 out:
	free resources
	return ret;

Noticed in the svn-dump-fast-export project, where using the error()
macro in void context produces a warning.

Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Changes since v2:
 - new description

 vcs-svn/svndiff.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index c89d9623..e810d0c3 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -258,6 +258,7 @@ static int apply_window_in_core(struct window *ctx)
 static int apply_one_window(struct line_buffer *delta, off_t *delta_len,
 			    struct sliding_view *preimage, FILE *out)
 {
+	int rv = -1;
 	struct window ctx = WINDOW_INIT(preimage);
 	size_t out_len;
 	size_t instructions_len;
@@ -275,16 +276,15 @@ static int apply_one_window(struct line_buffer *delta, off_t *delta_len,
 	if (apply_window_in_core(&ctx))
 		goto error_out;
 	if (ctx.out.len != out_len) {
-		error("invalid delta: incorrect postimage length");
+		rv = error("invalid delta: incorrect postimage length");
 		goto error_out;
 	}
 	if (write_strbuf(&ctx.out, out))
 		goto error_out;
-	window_release(&ctx);
-	return 0;
+	rv = 0;
 error_out:
 	window_release(&ctx);
-	return -1;
+	return rv;
 }
 
 int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
-- 
1.7.10.4

[PATCH 4/9] vcs-svn: use constcmp instead of prefixcmp

[Jonathan Nieder]

From: David Barr <davidbarr@google.com>
Date: Fri, 1 Jun 2012 00:41:27 +1000

Since the length of t is already known, we can simplify a little by
using memcmp() instead of strncmp() to carry out a prefix comparison.
All nearby code already does this.

Noticed in the standalone svn-dump-fast-export project which has not
needed to implement prefixcmp() yet.

Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Description clarified.  No other change.

 vcs-svn/svndump.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index f6c0d4c8..c5d07a66 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -361,7 +361,7 @@ void svndump_read(const char *url)
 			reset_rev_ctx(atoi(val));
 			break;
 		case sizeof("Node-path"):
-			if (prefixcmp(t, "Node-"))
+			if (constcmp(t, "Node-"))
 				continue;
 			if (!constcmp(t + strlen("Node-"), "path")) {
 				if (active_ctx == NODE_CTX)
-- 
1.7.10.4

[PATCH 5/9] vcs-svn: use strstr instead of memmem

[Jonathan Nieder]

From: David Barr <davidbarr@google.com>
Date: Fri, 1 Jun 2012 00:41:28 +1000

memmem is a GNU extension.

Avoiding it makes the code clearer and makes it easier for projects
that don't share git's compat/ code, such as the standalone
svn-dump-fast-export project, to reuse the vcs-svn/ library.

Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Clarified description.  No other change since v2.

 vcs-svn/fast_export.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index b4be91cc..854b328d 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -158,7 +158,7 @@ static int parse_cat_response_line(const char *header, off_t *len)
 
 	if (ends_with(header, headerlen, " missing"))
 		return error("cat-blob reports missing blob: %s", header);
-	type = memmem(header, headerlen, " blob ", strlen(" blob "));
+	type = strstr(header, " blob ");
 	if (!type)
 		return error("cat-blob header has wrong object type: %s", header);
 	n = strtoumax(type + strlen(" blob "), (char **) &end, 10);
-- 
1.7.10.4

[PATCH 6/9] vcs-svn: suppress signed/unsigned comparison warnings

[Jonathan Nieder]

From: David Barr <davidbarr@google.com>
Date: Fri, 1 Jun 2012 00:41:29 +1000

These are already safe because both sides of the comparison are
nonnegative.

This would normally not be important because Git is not -Wsign-compare
clean anyway, but we like to keep the vcs-svn/ lib to a higher
standard for convenience using it in other projects.

Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
This is the easy part of what is a larger patch[1] in v2, split out
and given a new description for easier review.

[1] http://thread.gmane.org/gmane.comp.version-control.git/198909/focus=198914

 vcs-svn/fast_export.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 854b328d..1f046978 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -254,7 +254,7 @@ static int parse_ls_response(const char *response, uint32_t *mode,
 	}
 
 	/* Mode. */
-	if (response_end - response < strlen("100644") ||
+	if (response_end - response < (signed) strlen("100644") ||
 	    response[strlen("100644")] != ' ')
 		die("invalid ls response: missing mode: %s", response);
 	*mode = 0;
@@ -267,7 +267,7 @@ static int parse_ls_response(const char *response, uint32_t *mode,
 	}
 
 	/* ' blob ' or ' tree ' */
-	if (response_end - response < strlen(" blob ") ||
+	if (response_end - response < (signed) strlen(" blob ") ||
 	    (response[1] != 'b' && response[1] != 't'))
 		die("unexpected ls response: not a tree or blob: %s", response);
 	response += strlen(" blob ");
-- 
1.7.10.4

[PATCH 7/9] vcs-svn: suppress a signed/unsigned comparison warning

[Jonathan Nieder]

From: David Barr <davidbarr@google.com>
Date: Fri, 1 Jun 2012 00:41:29 +1000

The preceding code checks that view->max_off is nonnegative and
(off + width) fits in an off_t, so this code is already safe.

Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Another unobjectionable piece from v2's patch 5.  The new change
description explains why it's unobjectionable to save future readers
some time.

 vcs-svn/sliding_window.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/vcs-svn/sliding_window.c b/vcs-svn/sliding_window.c
index ec2707c9..f11d4909 100644
--- a/vcs-svn/sliding_window.c
+++ b/vcs-svn/sliding_window.c
@@ -54,7 +54,7 @@ int move_window(struct sliding_view *view, off_t off, size_t width)
 		return -1;
 	if (off < view->off || off + width < view->off + view->width)
 		return error("invalid delta: window slides left");
-	if (view->max_off >= 0 && view->max_off < off + width)
+	if (view->max_off >= 0 && view->max_off < off + (off_t) width)
 		return error("delta preimage ends early");
 
 	file_offset = view->off + view->buf.len;
-- 
1.7.10.4

[PATCH 8/9] vcs-svn: suppress a signed/unsigned comparison warning

[Jonathan Nieder]

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Thu, 5 Jul 2012 22:21:09 -0500

All callers pass a nonnegative delta_len, so the code is already safe.
Add an assertion to ensure that remains so and add a cast to keep
clang and gcc -Wsign-compare from worrying.

Reported-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
v2 suppressed the warning by casting "len" to an off_t, producing an
unintentional change (breakage) in functionality on 64-bit systems
when "len" is large.

This version is longer but more conservative.

 vcs-svn/svndiff.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/vcs-svn/svndiff.c b/vcs-svn/svndiff.c
index e810d0c3..74c97c45 100644
--- a/vcs-svn/svndiff.c
+++ b/vcs-svn/svndiff.c
@@ -77,8 +77,9 @@ static int error_short_read(struct line_buffer *input)
 static int read_chunk(struct line_buffer *delta, off_t *delta_len,
 		      struct strbuf *buf, size_t len)
 {
+	assert(*delta_len >= 0);
 	strbuf_reset(buf);
-	if (len > *delta_len ||
+	if (len > (uintmax_t) *delta_len ||
 	    buffer_read_binary(delta, buf, len) != len)
 		return error_short_read(delta);
 	*delta_len -= buf->len;
@@ -290,7 +291,7 @@ error_out:
 int svndiff0_apply(struct line_buffer *delta, off_t delta_len,
 			struct sliding_view *preimage, FILE *postimage)
 {
-	assert(delta && preimage && postimage);
+	assert(delta && preimage && postimage && delta_len >= 0);
 
 	if (read_magic(delta, &delta_len))
 		return -1;
-- 
1.7.10.4

[PATCH 9/9] vcs-svn: allow 64-bit Prop-Content-Length

[Jonathan Nieder]

Date: Thu, 5 Jul 2012 22:47:47 -0500

Currently the vcs-svn/ library only pays attention to the presence of
the Prop-Content-Length field and doesn't care about its value, but
some day we might care about the value.  Parse it as an off_t instead
of arbitrarily limiting to 32 bits for intuitiveness.

So now you can import from a dump with more than 2 GiB of properties
for a node.  In practice that isn't likely to happen often, and this
is mostly meant as a cleanup.

Based-on-patch-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Another change that was mixed into v2's signedness warnings patch.
In v2 it changed the type of propLength without changing its name.
This version of the patch is more thorough about consistently using
the intuitive type (off_t instead of a 32-bit integer).

That's the end of the series.  Thanks for your patience.

 vcs-svn/svndump.c |   33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index c5d07a66..a7f3ea64 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -34,14 +34,13 @@
 #define NODE_CTX 2	/* node metadata */
 #define INTERNODE_CTX 3	/* between nodes */
 
-#define LENGTH_UNKNOWN (~0)
 #define DATE_RFC2822_LEN 31
 
 static struct line_buffer input = LINE_BUFFER_INIT;
 
 static struct {
-	uint32_t action, propLength, srcRev, type;
-	off_t text_length;
+	uint32_t action, srcRev, type;
+	off_t prop_length, text_length;
 	struct strbuf src, dst;
 	uint32_t text_delta, prop_delta;
 } node_ctx;
@@ -61,7 +60,7 @@ static void reset_node_ctx(char *fname)
 {
 	node_ctx.type = 0;
 	node_ctx.action = NODEACT_UNKNOWN;
-	node_ctx.propLength = LENGTH_UNKNOWN;
+	node_ctx.prop_length = -1;
 	node_ctx.text_length = -1;
 	strbuf_reset(&node_ctx.src);
 	node_ctx.srcRev = 0;
@@ -209,7 +208,7 @@ static void read_props(void)
 static void handle_node(void)
 {
 	const uint32_t type = node_ctx.type;
-	const int have_props = node_ctx.propLength != LENGTH_UNKNOWN;
+	const int have_props = node_ctx.prop_length != -1;
 	const int have_text = node_ctx.text_length != -1;
 	/*
 	 * Old text for this node:
@@ -273,7 +272,7 @@ static void handle_node(void)
 	if (have_props) {
 		if (!node_ctx.prop_delta)
 			node_ctx.type = type;
-		if (node_ctx.propLength)
+		if (node_ctx.prop_length)
 			read_props();
 	}
 
@@ -409,22 +408,26 @@ void svndump_read(const char *url)
 			node_ctx.srcRev = atoi(val);
 			break;
 		case sizeof("Text-content-length"):
-			if (!constcmp(t, "Text-content-length")) {
+			if (constcmp(t, "Text") && constcmp(t, "Prop"))
+				continue;
+			if (constcmp(t + 4, "-content-length"))
+				continue;
+			{
 				char *end;
-				uintmax_t textlen;
+				uintmax_t len;
 
-				textlen = strtoumax(val, &end, 10);
+				len = strtoumax(val, &end, 10);
 				if (!isdigit(*val) || *end)
 					die("invalid dump: non-numeric length %s", val);
-				if (textlen > maximum_signed_value_of_type(off_t))
+				if (len > maximum_signed_value_of_type(off_t))
 					die("unrepresentable length in dump: %s", val);
-				node_ctx.text_length = (off_t) textlen;
+
+				if (*t == 'T')
+					node_ctx.text_length = (off_t) len;
+				else
+					node_ctx.prop_length = (off_t) len;
 				break;
 			}
-			if (constcmp(t, "Prop-content-length"))
-				continue;
-			node_ctx.propLength = atoi(val);
-			break;
 		case sizeof("Text-delta"):
 			if (!constcmp(t, "Text-delta")) {
 				node_ctx.text_delta = !strcmp(val, "true");
-- 
1.7.10.4

Re: [GIT PULL] vcs-svn housekeeping

[David Michael Barr]

On Sat, Jul 7, 2012 at 3:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi Junio,
>
> The following changes since commit 58ebd9865d2bb9d42842fbac5a1c4eae49e92859:
>
>   vcs-svn/svndiff.c: squelch false "unused" warning from gcc (2012-01-27 11:58:56 -0800)
>
> are available at:
>
>   git://repo.or.cz/git/jrn.git svn-fe
>
> The first three commits duplicate changes that are already in master
> but were committed independently on the svn-fe branch last February.
> The rest are David's db/vcs-svn series which aims to address various
> nits noticed when merging the code back into svn-dump-fast-export:
> unnecessary use of git-specific functions (prefixcmp, memmem) and
> warnings reported by clang.
>
> Some of the patches had to change a little since v2 of db/vcs-svn, so
> I'll be replying with a copy of the patches for reference.
>
> David has looked the branch over and acked and tested it.
>
> Thoughts welcome, as usual.  I think these are ready for pulling into
> master.  Sorry to be so slow at this.
>
> David Barr (7):
>       vcs-svn: drop no-op reset methods
>       vcs-svn: avoid self-assignment in dummy initialization of pre_off
>       vcs-svn: simplify cleanup in apply_one_window
>       vcs-svn: use constcmp instead of prefixcmp
>       vcs-svn: use strstr instead of memmem
>       vcs-svn: suppress signed/unsigned comparison warnings
>       vcs-svn: suppress a signed/unsigned comparison warning
>
> Jonathan Nieder (4):
>       vcs-svn: allow import of > 4GiB files
>       vcs-svn: suppress -Wtype-limits warning
>       vcs-svn: suppress a signed/unsigned comparison warning
>       vcs-svn: allow 64-bit Prop-Content-Length
>
> Ramsay Allan Jones (1):
>       vcs-svn: rename check_overflow and its arguments for clarity

Thank you Jonathan for doing this. Definitely the result of
collaborating on a series is gorgeous. I do wish I could absorb your
flair for polish.

--
David Barr

Re: [GIT PULL] vcs-svn housekeeping

[Junio C Hamano]

David Michael Barr <davidbarr@google.com> writes:

> On Sat, Jul 7, 2012 at 3:10 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> ...
>> Some of the patches had to change a little since v2 of db/vcs-svn, so
>> I'll be replying with a copy of the patches for reference.
>>
>> David has looked the branch over and acked and tested it.
>> ...
>
> Thank you Jonathan for doing this. Definitely the result of
> collaborating on a series is gorgeous. I do wish I could absorb your
> flair for polish.

Thanks, both.  Will be pulled and pushed out by tonight.