* [PATCH 2/3] format-patch documentation: Remove diff options that are not useful
From: Björn Gustavsson @ 2009-10-25 15:56 UTC (permalink / raw)
To: git; +Cc: gitster
To simplify reading the documentation for format-patch, remove the
description of common diff options that are not useful for the
purpose of the command (i.e. "Prepare patches for e-mail submission").
Remove the description of the following options:
--raw
-z
--color
--no-color
--color-words
--diff-filter
-S
--pickaxe-all
--pickaxe-regex
-R
--relative
--exit-code
--quiet
---
Documentation/diff-options.txt | 15 ++++++++++++---
1 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 673fbb0..88e88d7 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -27,9 +27,11 @@ ifndef::git-format-patch[]
Implies "-p".
endif::git-format-patch[]
+ifndef::git-format-patch[]
--raw::
Generate the raw format.
{git-diff-core? This is the default.}
+endif::git-format-patch[]
ifndef::git-format-patch[]
--patch-with-raw::
@@ -76,20 +78,18 @@ ifndef::git-format-patch[]
Synonym for "-p --stat".
endif::git-format-patch[]
+ifndef::git-format-patch[]
-z::
NUL-line termination on output. This affects the --raw
output field terminator. Also output from commands such
as "git-log" will be delimited with NUL between commits.
-ifndef::git-format-patch[]
--name-only::
Show only names of changed files.
--name-status::
Show only names and status of changed files. See the description
of the `--diff-filter` option on what the status letters mean.
-endif::git-format-patch[]
-
--color::
Show colored diff.
@@ -113,6 +113,7 @@ The regex can also be set via a diff driver or configuration option, see
linkgit:gitattributes[1] or linkgit:git-config[1]. Giving it explicitly
overrides any diff driver or configuration setting. Diff drivers
override configuration settings.
+endif::git-format-patch[]
--no-renames::
Turn off rename detection, even when the configuration
@@ -152,6 +153,7 @@ endif::git-format-patch[]
-C::
Detect copies as well as renames. See also `--find-copies-harder`.
+ifndef::git-format-patch[]
--diff-filter=[ACDMRTUXB*]::
Select only files that are Added (`A`), Copied (`C`),
Deleted (`D`), Modified (`M`), Renamed (`R`), have their
@@ -163,6 +165,7 @@ endif::git-format-patch[]
paths are selected if there is any file that matches
other criteria in the comparison; if there is no file
that matches other criteria, nothing is selected.
+endif::git-format-patch[]
--find-copies-harder::
For performance reasons, by default, `-C` option finds copies only
@@ -180,6 +183,7 @@ endif::git-format-patch[]
the number of rename/copy targets exceeds the specified
number.
+ifndef::git-format-patch[]
-S<string>::
Look for differences that introduce or remove an instance of
<string>. Note that this is different than the string simply
@@ -194,11 +198,13 @@ endif::git-format-patch[]
--pickaxe-regex::
Make the <string> not a plain string but an extended POSIX
regex to match.
+endif::git-format-patch[]
-O<orderfile>::
Output the patch in the order specified in the
<orderfile>, which has one shell glob pattern per line.
+ifndef::git-format-patch[]
-R::
Swap two inputs; that is, show differences from index or
on-disk file to tree contents.
@@ -210,6 +216,7 @@ endif::git-format-patch[]
not in a subdirectory (e.g. in a bare repository), you
can name which subdirectory to make the output relative
to by giving a <path> as an argument.
+endif::git-format-patch[]
-a::
--text::
@@ -234,6 +241,7 @@ endif::git-format-patch[]
Show the context between diff hunks, up to the specified number
of lines, thereby fusing hunks that are close to each other.
+ifndef::git-format-patch[]
--exit-code::
Make the program exit with codes similar to diff(1).
That is, it exits with 1 if there were differences and
@@ -241,6 +249,7 @@ endif::git-format-patch[]
--quiet::
Disable all output of the program. Implies --exit-code.
+endif::git-format-patch[]
--ext-diff::
Allow an external diff helper to be executed. If you set an
--
1.6.5.1.69.g36942
^ permalink raw reply related
* [PATCH 1/3] format-patch: Make implementation and documentation agree
From: Björn Gustavsson @ 2009-10-25 15:55 UTC (permalink / raw)
To: git; +Cc: gitster
The documentation for the -p option for format-patch says:
"Generate patches without diffstat."
Starting with commit 68daa64d, however, -p no longer suppresses the
diffstat and is a no-op *most* of the time. It is still needed when
--stat (or one of the other 'stat' options) is given in order to
produce a patch at all.
Since no-one seems to have noticed (or cared) that -p no longer
suppresses the diffstat (after more than a year), it makes most
sense to fix the documentation (rather than the implementation).
The trouble is that -p is still sometimes needed, so to simplify
the documentation, it would be best to change the implementation
as well.
Therefore:
* Update 'git format-patch' to always generate a patch.
* Since the --name-only, --name-status, and --check suppresses
the generation of the patch, disallow those options. They don't
make sense for format-patch anyway.
* Remove the description of -p from the documentation.
* Remove the reference to -p in the description of -U.
* Remove the descriptions of the options that are synonyms for -p
plus another option (--patch-with-raw and --patch-with-stat).
* Remove the description of the options that are no longer
allowed (--name-only, --name-status, and --check).
---
Documentation/diff-options.txt | 19 ++++++++++++-------
builtin-log.c | 12 +++++++++++-
t/t4014-format-patch.sh | 18 ++++++++++++++++++
3 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 9276fae..673fbb0 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -12,11 +12,6 @@ endif::git-log[]
endif::git-diff[]
endif::git-format-patch[]
-ifdef::git-format-patch[]
--p::
- Generate patches without diffstat.
-endif::git-format-patch[]
-
ifndef::git-format-patch[]
-p::
-u::
@@ -27,14 +22,19 @@ endif::git-format-patch[]
-U<n>::
--unified=<n>::
Generate diffs with <n> lines of context instead of
- the usual three. Implies "-p".
+ the usual three.
+ifndef::git-format-patch[]
+ Implies "-p".
+endif::git-format-patch[]
--raw::
Generate the raw format.
{git-diff-core? This is the default.}
+ifndef::git-format-patch[]
--patch-with-raw::
Synonym for "-p --raw".
+endif::git-format-patch[]
--patience::
Generate a diff using the "patience diff" algorithm.
@@ -71,21 +71,24 @@ endif::git-format-patch[]
Output a condensed summary of extended header information
such as creations, renames and mode changes.
+ifndef::git-format-patch[]
--patch-with-stat::
Synonym for "-p --stat".
- {git-format-patch? This is the default.}
+endif::git-format-patch[]
-z::
NUL-line termination on output. This affects the --raw
output field terminator. Also output from commands such
as "git-log" will be delimited with NUL between commits.
+ifndef::git-format-patch[]
--name-only::
Show only names of changed files.
--name-status::
Show only names and status of changed files. See the description
of the `--diff-filter` option on what the status letters mean.
+endif::git-format-patch[]
--color::
Show colored diff.
@@ -115,11 +118,13 @@ override configuration settings.
Turn off rename detection, even when the configuration
file gives the default to do so.
+ifndef::git-format-patch[]
--check::
Warn if changes introduce trailing whitespace
or an indent that uses a space before a tab. Exits with
non-zero status if problems are found. Not compatible with
--exit-code.
+endif::git-format-patch[]
--full-index::
Instead of the first handful of characters, show the full
diff --git a/builtin-log.c b/builtin-log.c
index 25e21ed..7b2cde2 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1027,9 +1027,19 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
if (argc > 1)
die ("unrecognized argument: %s", argv[1]);
+ if (rev.diffopt.output_format & DIFF_FORMAT_NAME)
+ die("--name-only does not make sense");
+ if (rev.diffopt.output_format & DIFF_FORMAT_NAME_STATUS)
+ die("--name-status does not make sense");
+ if (rev.diffopt.output_format & DIFF_FORMAT_CHECKDIFF)
+ die("--check does not make sense");
+
if (!rev.diffopt.output_format
|| rev.diffopt.output_format == DIFF_FORMAT_PATCH)
- rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY | DIFF_FORMAT_PATCH;
+ rev.diffopt.output_format = DIFF_FORMAT_DIFFSTAT | DIFF_FORMAT_SUMMARY;
+
+ /* Always generate a patch */
+ rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
if (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)
DIFF_OPT_SET(&rev.diffopt, BINARY);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 531f5b7..f826348 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -515,4 +515,22 @@ test_expect_success 'format-patch --signoff' '
grep "^Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
'
+echo "fatal: --name-only does not make sense" > expect.name-only
+echo "fatal: --name-status does not make sense" > expect.name-status
+echo "fatal: --check does not make sense" > expect.check
+
+test_expect_success 'options no longer allowed for format-patch' '
+ test_must_fail git format-patch --name-only 2> output &&
+ test_cmp expect.name-only output &&
+ test_must_fail git format-patch --name-status 2> output &&
+ test_cmp expect.name-status output &&
+ test_must_fail git format-patch --check 2> output &&
+ test_cmp expect.check output'
+
+test_expect_success 'format-patch --numstat should produce a patch' '
+ git format-patch --numstat --stdout master..side |
+ grep "^diff --git a/" |
+ wc -l |
+ xargs test 6 = '
+
test_done
--
1.6.5.1.69.g36942
^ permalink raw reply related
* [PATCH 0/3] format-patch updates
From: Björn Gustavsson @ 2009-10-25 15:54 UTC (permalink / raw)
To: git; +Cc: gitster
I noticed that the description of the -p option for the format-patch was out-of-date.
In the first patch I suggest how the documentation and implementation can
again be made to agree.
The second patch builds on the first patch and removes the description of
common diff options that are not useful for format-patch (AFAIK), to make the
documentation easier to read.
The third patch cleans up formatting of git commands and options.
Björn Gustavsson (3):
format-patch: Make implementation and documentation agree
format-patch documentation: Remove diff options that are not useful
format-patch documentation: Fix formatting
Documentation/diff-options.txt | 54 ++++++++++++++++++++++-------------
Documentation/git-format-patch.txt | 44 ++++++++++++++--------------
builtin-log.c | 12 +++++++-
t/t4014-format-patch.sh | 18 ++++++++++++
4 files changed, 85 insertions(+), 43 deletions(-)
^ permalink raw reply
* [PATCH 7/7] t5540-http-push: remove redundant fetches
From: Tay Ray Chuan @ 2009-10-25 15:24 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <20091025232310.2c512a1b.rctay89@gmail.com>
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
t/t5540-http-push.sh | 2 --
1 files changed, 0 insertions(+), 2 deletions(-)
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 6bb5afa..3852dcb 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -114,9 +114,7 @@ test_expect_success 'create and delete remote branch' '
test_tick &&
git commit -m dev &&
git push origin dev &&
- git fetch &&
git push origin :dev &&
- git fetch &&
test_must_fail git show-ref --verify refs/remotes/origin/dev
'
--
1.6.4.4
^ permalink raw reply related
* [PATCH 6/7] t5540-http-push: when deleting remote refs, don't need to branch -d -r
From: Tay Ray Chuan @ 2009-10-25 15:23 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <20091025232227.96769e50.rctay89@gmail.com>
In c6aa27e (Move WebDAV HTTP push under remote-curl, Wed Oct 14),
transport->push_refs became defined for http pushing. This enabled the
code to update local tracking refs (transport.c::update_tracking_ref()
via transport.c::transport_push()), allowing these refs to be deleted
together with the remote ref, thus doing away with a manual branch -d
-r.
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
t/t5540-http-push.sh | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index ee7f84a..6bb5afa 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -116,7 +116,6 @@ test_expect_success 'create and delete remote branch' '
git push origin dev &&
git fetch &&
git push origin :dev &&
- git branch -d -r origin/dev &&
git fetch &&
test_must_fail git show-ref --verify refs/remotes/origin/dev
'
--
1.6.4.4
^ permalink raw reply related
* [PATCH 5/7] t5540-http-push: check existence of fetched files
From: Tay Ray Chuan @ 2009-10-25 15:22 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <20091025232142.6558d9e4.rctay89@gmail.com>
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
t/t5540-http-push.sh | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index c7b8a40..ee7f84a 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -64,13 +64,18 @@ test_expect_success 'http-push fetches unpacked objects' '
git clone $HTTPD_URL/test_repo_unpacked.git \
"$ROOT_PATH"/fetch_unpacked &&
+ COMMIT_PATH=$(git rev-parse --verify HEAD |
+ sed -e "s/^\([0-9a-f]\{2\}\)\([0-9a-f]\{38\}\)/\1\/\2/") &&
+
# By reset, we force git to retrieve the object
(cd "$ROOT_PATH"/fetch_unpacked &&
git reset --hard HEAD^ &&
git remote rm origin &&
git reflog expire --expire=0 --all &&
git prune &&
- git push -f -v $HTTPD_URL/test_repo_unpacked.git master)
+ test ! -e ".git/objects/$COMMIT_PATH" &&
+ git push -f -v $HTTPD_URL/test_repo_unpacked.git master &&
+ test -e ".git/objects/$COMMIT_PATH")
'
test_expect_success 'http-push fetches packed objects' '
@@ -80,9 +85,14 @@ test_expect_success 'http-push fetches packed objects' '
git clone $HTTPD_URL/test_repo_packed.git \
"$ROOT_PATH"/test_repo_clone_packed &&
- (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_packed.git &&
- git --bare repack &&
- git --bare prune-packed) &&
+ cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_packed.git &&
+ git --bare repack &&
+ git --bare prune-packed &&
+
+ PACK_PATH=$(cat "objects/info/packs" |
+ sed -e "s/^P \(pack-[0-9a-f]\{40\}\.pack\)/\1/") &&
+ COMMIT_PATH=$(git rev-parse --verify HEAD |
+ sed -e "s/^\([0-9a-f]\{2\}\)\([0-9a-f]\{38\}\)/\1\/\2/") &&
# By reset, we force git to retrieve the packed object
(cd "$ROOT_PATH"/test_repo_clone_packed &&
@@ -90,7 +100,10 @@ test_expect_success 'http-push fetches packed objects' '
git remote rm origin &&
git reflog expire --expire=0 --all &&
git prune &&
- git push -f -v $HTTPD_URL/test_repo_packed.git master)
+ test ! -e ".git/objects/$COMMIT_PATH" &&
+ test ! -e ".git/objects/pack/$PACK_PATH" &&
+ git push -f -v $HTTPD_URL/test_repo_packed.git master &&
+ test -e ".git/objects/pack/$PACK_PATH")
'
test_expect_success 'create and delete remote branch' '
--
1.6.4.4
^ permalink raw reply related
* [PATCH 4/7] t5540-http-push: expect success when pushing without arguments
From: Tay Ray Chuan @ 2009-10-25 15:21 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <20091025232044.06d7ce5c.rctay89@gmail.com>
Remove mention of packed refs from the test description, and expect
success.
The dumb http push mechanism used to learn about the refs on the remote
repository by recursing through the /refs directory in the repository.
This meant that it was unaware of packed refs, since it did not read
/packed-refs. Thus the push failed, as no remote refs were found.
But after c6aa27e (Move WebDAV HTTP push under remote-curl, Wed Oct
14), the dumb http mechanism additionally learns about the refs through
/info/refs (via remote-curl.c::get_refs), so it is aware of packed
refs, even though it still doesn't read /packed-refs (assuming /info/
refs is up-to-date). Thus the push now succeeds.
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
t/t5540-http-push.sh | 12 +-----------
1 files changed, 1 insertions(+), 11 deletions(-)
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index f4a2cf6..c7b8a40 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -45,7 +45,7 @@ test_expect_success 'clone remote repository' '
git clone $HTTPD_URL/test_repo.git test_repo_clone
'
-test_expect_failure 'push to remote repository with packed refs' '
+test_expect_success 'push to remote repository' '
cd "$ROOT_PATH"/test_repo_clone &&
: >path2 &&
git add path2 &&
@@ -57,16 +57,6 @@ test_expect_failure 'push to remote repository with packed refs' '
test $HEAD = $(git rev-parse --verify HEAD))
'
-test_expect_success ' push to remote repository with unpacked refs' '
- (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
- rm packed-refs &&
- git update-ref refs/heads/master \
- 0c973ae9bd51902a28466f3850b543fa66a6aaf4) &&
- git push &&
- (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
- test $HEAD = $(git rev-parse --verify HEAD))
-'
-
test_expect_success 'http-push fetches unpacked objects' '
cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
"$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_unpacked.git &&
--
1.6.4.4
^ permalink raw reply related
* [PATCH 3/7] http-push: add more 'error <dst> <why>' status reports
From: Tay Ray Chuan @ 2009-10-25 15:20 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <20091025231932.be9a6dfa.rctay89@gmail.com>
Complement c6aa27e (Move WebDAV HTTP push under remote-curl, Wed Oct
14) by adding error reports for 'cannot remove' (failed to delete
branch) and 'no match'.
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
http-push.c | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)
diff --git a/http-push.c b/http-push.c
index b97ea1f..f10803a 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1916,9 +1916,12 @@ int main(int argc, char **argv)
/* Remove a remote branch if -d or -D was specified */
if (delete_branch) {
- if (delete_remote_branch(refspec[0], force_delete) == -1)
+ if (delete_remote_branch(refspec[0], force_delete) == -1) {
fprintf(stderr, "Unable to delete remote branch %s\n",
refspec[0]);
+ if (helper_status)
+ printf("error %s cannot remove\n", refspec[0]);
+ }
goto cleanup;
}
@@ -1930,6 +1933,8 @@ int main(int argc, char **argv)
}
if (!remote_refs) {
fprintf(stderr, "No refs in common and none specified; doing nothing.\n");
+ if (helper_status)
+ printf("error null no match\n");
rc = 0;
goto cleanup;
}
--
1.6.4.4
^ permalink raw reply related
* [PATCH 2/7] http-push: allow stderr messages to appear alongside helper_status ones
From: Tay Ray Chuan @ 2009-10-25 15:19 UTC (permalink / raw)
To: git; +Cc: Shawn O. Pearce
In-Reply-To: <20091025231651.18c75559.rctay89@gmail.com>
These messages notifies user over pushing progress (or the lack
thereof). It is safe for them to appear alongside status reports
('ok|error <dst>'), since the reports go to stdout, while the
notifications go to stderr.
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
http-push.c | 21 +++++++++------------
1 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/http-push.c b/http-push.c
index 24eec73..b97ea1f 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1946,10 +1946,9 @@ int main(int argc, char **argv)
if (is_null_sha1(ref->peer_ref->new_sha1)) {
if (delete_remote_branch(ref->name, 1) == -1) {
+ error("Could not remove %s", ref->name);
if (helper_status)
printf("error %s cannot remove\n", ref->name);
- else
- error("Could not remove %s", ref->name);
rc = -4;
}
else if (helper_status)
@@ -1980,15 +1979,14 @@ int main(int argc, char **argv)
* commits at the remote end and likely
* we were not up to date to begin with.
*/
+ error("remote '%s' is not an ancestor of\n"
+ "local '%s'.\n"
+ "Maybe you are not up-to-date and "
+ "need to pull first?",
+ ref->name,
+ ref->peer_ref->name);
if (helper_status)
printf("error %s non-fast forward\n", ref->name);
- else
- error("remote '%s' is not an ancestor of\n"
- "local '%s'.\n"
- "Maybe you are not up-to-date and "
- "need to pull first?",
- ref->name,
- ref->peer_ref->name);
rc = -2;
continue;
}
@@ -2011,11 +2009,10 @@ int main(int argc, char **argv)
/* Lock remote branch ref */
ref_lock = lock_remote(ref->name, LOCK_TIME);
if (ref_lock == NULL) {
+ fprintf(stderr, "Unable to lock remote branch %s\n",
+ ref->name);
if (helper_status)
printf("error %s lock error\n", ref->name);
- else
- fprintf(stderr, "Unable to lock remote branch %s\n",
- ref->name);
rc = 1;
continue;
}
--
1.6.4.4
^ permalink raw reply related
* [PATCH 1/7] http-push: fix check condition on http.c::finish_http_pack_request()
From: Tay Ray Chuan @ 2009-10-25 15:18 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shawn O. Pearce
In-Reply-To: <20091025231651.18c75559.rctay89@gmail.com>
Check that http.c::finish_http_pack_request() returns 0 (for success).
Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---
http-push.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/http-push.c b/http-push.c
index 9010ccc..24eec73 100644
--- a/http-push.c
+++ b/http-push.c
@@ -605,7 +605,7 @@ static void finish_request(struct transfer_request *request)
preq = (struct http_pack_request *)request->userData;
if (preq) {
- if (finish_http_pack_request(preq) > 0)
+ if (finish_http_pack_request(preq) == 0)
fail = 0;
release_http_pack_request(preq);
}
--
1.6.4.4
^ permalink raw reply related
* [PATCH 0/6] http: push and test fixes
From: Tay Ray Chuan @ 2009-10-25 15:16 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Shawn O. Pearce
In-Reply-To: <1255577814-14745-1-git-send-email-spearce@spearce.org>
This series applies on top of pu, and should apply cleanly on top of
Shawn's v3-smart-http branch.
Patch 1: fixes a bug in http-push.c when fetching packs. This issue is
minor and is unlikely to be triggered by normal users (for a possible
trigger, see the "http-push fetches packed objects" test in
t5540-http-push).
Patch 2: here, I collect my comments to Shawn's "Move WebDAV HTTP push
under remote-curl" on the disabling of messages to stderr when
--helper-status is used with http-push. I hope this will make it
easier for Shawn to squash into his series, in the event he decides
to.
Patch 3: also in response to Shawn's "Move WebDAV HTTP push under
remote-curl", I collect here my mini-patch to add more reports for
transport-helper. Again, I hope this will make it easier for Shawn to
squash into his series.
Patch 4-7: update tests, as Shawn's patches changed the behaviour of
the http push mechanism (dumb).
Tay Ray Chuan (7):
http-push: fix check condition on http.c::finish_http_pack_request()
http-push: allow stderr messages to appear alongside helper_status
ones
http-push: add more 'error <dst> <why>' status reports
t5540-http-push: expect success when pushing without arguments
t5540-http-push: check existence of fetched files
t5540-http-push: when deleting remote refs, don't need to branch -d
-r
t5540-http-push: remove redundant fetches
http-push.c | 30 ++++++++++++++++--------------
t/t5540-http-push.sh | 38 +++++++++++++++++++-------------------
2 files changed, 35 insertions(+), 33 deletions(-)
^ permalink raw reply
* Re: My custom cccmd
From: Felipe Contreras @ 2009-10-25 15:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <94a0d4530910151437s780bd96anca82d2b26ef99e0a@mail.gmail.com>
On Thu, Oct 15, 2009 at 11:37 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Thu, Oct 15, 2009 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> #2. If you have two patch series that updates one file twice, some
>> changes in your second patch could even be an update to the changes
>> you introduced in your first patch. After you fix issue #1, you
>> would probably want to fix this by excluding the commits you have
>> already sent the blames for.
>
> How exactly? By looking at the commit from 'git blame' and discarding
> it? That would be a bit tricky since each instance of 'cccmd' is not
> aware of the previous ones.
I explored this a bit more and I've come to the conclusion that we
actually don't wand to discard the previous commits in the patch
series. Let's think about this example:
0001 <- John
0002 <- Me overriding some changes from John
In this case we want John to appear in the CC list of 0002, because we
are changing his code. If it turns out that the whole patch series was
written by me, it's still ok for me to appear on the CC list, which
would essentially be "discarding" those commits.
So all we need to do is pick @from^ as the base of the git blame:
@@ -17,11 +17,12 @@ end
ARGV.each do |filename|
patch_file = File.open(filename)
patch_file.each_line do |patch_line|
+ @from ||= patch_line[/From (\w+)/, 1]
case patch_line
when /^---\s+(\S+)/
@source = $1[2..-1]
when /^@@\s-(\d+),(\d+)/
- blame = `git blame -p -L #{$1},+#{$2} #{@source} | grep author`
+ blame = `git blame --incremental -L #{$1},+#{$2} #{@source}
#{@from}^ | grep author`
blame.each_line { |l| parse_blame(l.chomp) }
end
end
You can find the whole script here:
http://gist.github.com/218093
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [ANNOUNCE] BugTracker.NET (FOSS) now supports git integration
From: Michael Poole @ 2009-10-25 14:03 UTC (permalink / raw)
To: Corey Trager; +Cc: git
In-Reply-To: <97625.96130.qm@web65401.mail.ac4.yahoo.com>
Corey Trager writes:
> BugTracker.NET is a free, open-source, GPL, ASP.NET bug tracking app.
> More info at http://ifdefined.com/bugtrackernet.html
>
> With the integration, if you do a commit like...
> git commit -a -m "123 fixed the bug"
> ...then the hook script will link up the commit to bug 123.
>
> Here are screenshots of the Subversion integration, which looks pretty much like the git integration:
> http://ifdefined.com/doc_bug_tracker_subversion.html
>
> Feedback very welcome, good or bad.
Does it recognize bug IDs in the footer section of a commit message
(where Signed-Off-By and similar lines typically go)?
Michael Poole
^ permalink raw reply
* [ANNOUNCE] BugTracker.NET (FOSS) now supports git integration
From: Corey Trager @ 2009-10-25 13:15 UTC (permalink / raw)
To: git
BugTracker.NET is a free, open-source, GPL, ASP.NET bug tracking app.
More info at http://ifdefined.com/bugtrackernet.html
With the integration, if you do a commit like...
git commit -a -m "123 fixed the bug"
...then the hook script will link up the commit to bug 123.
Here are screenshots of the Subversion integration, which looks pretty much like the git integration:
http://ifdefined.com/doc_bug_tracker_subversion.html
Feedback very welcome, good or bad.
^ permalink raw reply
* [PATCH 3/3] add smart HTTP tests
From: Clemens Buchacher @ 2009-10-25 12:06 UTC (permalink / raw)
To: spearce; +Cc: Mark Lodato, git, Clemens Buchacher
In-Reply-To: <1256472380-924-3-git-send-email-drizzd@aon.at>
Set LIB_HTTPD_GIT to enable smart HTTP tests.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
t/lib-httpd.sh | 9 +++++++++
t/lib-httpd/apache.conf | 36 +++++++++++++++++++++++++++---------
t/t5540-http-push.sh | 46 ++++++++++++++++++++++++++++------------------
3 files changed, 64 insertions(+), 27 deletions(-)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 6765b08..aaa0a71 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -88,6 +88,15 @@ prepare_httpd() {
svnrepo="http://127.0.0.1:$LIB_HTTPD_PORT/svn"
fi
fi
+
+ if test -n "$LIB_HTTPD_GIT"
+ then
+ HTTPD_PARA="$HTTPD_PARA -DGIT"
+ fi
+
+ HTTPD_GIT_PATH=$HTTPD_DOCUMENT_ROOT_PATH/git
+ mkdir -p "$HTTPD_GIT_PATH"
+ HTTPD_GIT_URL="$HTTPD_URL/git"
}
start_httpd() {
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 21aa42f..d8505f1 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -10,15 +10,15 @@ ErrorLog error.log
</IfModule>
<IfDefine SSL>
-LoadModule ssl_module modules/mod_ssl.so
-
-SSLCertificateFile httpd.pem
-SSLCertificateKeyFile httpd.pem
-SSLRandomSeed startup file:/dev/urandom 512
-SSLRandomSeed connect file:/dev/urandom 512
-SSLSessionCache none
-SSLMutex file:ssl_mutex
-SSLEngine On
+ LoadModule ssl_module modules/mod_ssl.so
+
+ SSLCertificateFile httpd.pem
+ SSLCertificateKeyFile httpd.pem
+ SSLRandomSeed startup file:/dev/urandom 512
+ SSLRandomSeed connect file:/dev/urandom 512
+ SSLSessionCache none
+ SSLMutex file:ssl_mutex
+ SSLEngine On
</IfDefine>
<IfDefine DAV>
@@ -39,3 +39,21 @@ SSLEngine On
SVNPath svnrepo
</Location>
</IfDefine>
+
+<IfDefine GIT>
+ LoadModule cgi_module modules/mod_cgi.so
+ LoadModule alias_module modules/mod_alias.so
+ LoadModule actions_module modules/mod_actions.so
+ ScriptAlias /git-http-backend ${GIT_EXEC_PATH}/git-http-backend
+
+ <Location /git>
+ SetHandler git-http-backend
+ Action git-http-backend /git-http-backend virtual
+ </Location>
+ <Directory ${GIT_EXEC_PATH}>
+ Options None
+ </Directory>
+ <Files ${GIT_EXEC_PATH}/git-http-backend>
+ Options ExecCGI
+ </Files>
+</IfDefine>
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 2ece661..74ca08d 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -10,7 +10,10 @@ This test runs various sanity checks on http-push.'
. ./test-lib.sh
ROOT_PATH="$PWD"
-LIB_HTTPD_DAV=t
+if test -z "$LIB_HTTPD_GIT"
+then
+ LIB_HTTPD_DAV=t
+fi
LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5540'}
if git http-push > /dev/null 2>&1 || [ $? -eq 128 ]
@@ -34,16 +37,21 @@ test_expect_success 'setup remote repository' '
cd - &&
git clone --bare test_repo test_repo.git &&
cd test_repo.git &&
- git --bare update-server-info &&
- mv hooks/post-update.sample hooks/post-update &&
ORIG_HEAD=$(git rev-parse --verify HEAD) &&
- cd - &&
- mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
+ if test -n "$LIB_HTTPD_GIT"
+ then
+ git config http.receivepack true
+ else
+ git --bare update-server-info &&
+ mv hooks/post-update.sample hooks/post-update
+ fi
+ cd -
+ mv test_repo.git "$HTTPD_GIT_PATH"
'
test_expect_success 'clone remote repository' '
cd "$ROOT_PATH" &&
- git clone $HTTPD_URL/test_repo.git test_repo_clone
+ git clone $HTTPD_GIT_URL/test_repo.git test_repo_clone
'
test_expect_success 'push to remote repository with packed refs' '
@@ -54,7 +62,7 @@ test_expect_success 'push to remote repository with packed refs' '
git commit -m path2 &&
HEAD=$(git rev-parse --verify HEAD) &&
git push &&
- (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
+ (cd "$HTTPD_GIT_PATH"/test_repo.git &&
test $HEAD = $(git rev-parse --verify HEAD))
'
@@ -63,20 +71,20 @@ test_expect_success 'push already up-to-date' '
'
test_expect_success 'push to remote repository with unpacked refs' '
- (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
+ (cd "$HTTPD_GIT_PATH"/test_repo.git &&
rm packed-refs &&
git update-ref refs/heads/master $ORIG_HEAD &&
git --bare update-server-info) &&
git push &&
- (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
+ (cd "$HTTPD_GIT_PATH"/test_repo.git &&
test $HEAD = $(git rev-parse --verify HEAD))
'
test_expect_success 'http-push fetches unpacked objects' '
- cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
- "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_unpacked.git &&
+ cp -R "$HTTPD_GIT_PATH"/test_repo.git \
+ "$HTTPD_GIT_PATH"/test_repo_unpacked.git &&
- git clone $HTTPD_URL/test_repo_unpacked.git \
+ git clone $HTTPD_GIT_URL/test_repo_unpacked.git \
"$ROOT_PATH"/fetch_unpacked &&
# By reset, we force git to retrieve the object
@@ -85,17 +93,17 @@ test_expect_success 'http-push fetches unpacked objects' '
git remote rm origin &&
git reflog expire --expire=0 --all &&
git prune &&
- git push -f -v $HTTPD_URL/test_repo_unpacked.git master)
+ git push -f -v $HTTPD_GIT_URL/test_repo_unpacked.git master)
'
test_expect_success 'http-push fetches packed objects' '
- cp -R "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git \
- "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_packed.git &&
+ cp -R "$HTTPD_GIT_PATH"/test_repo.git \
+ "$HTTPD_GIT_PATH"/test_repo_packed.git &&
- git clone $HTTPD_URL/test_repo_packed.git \
+ git clone $HTTPD_GIT_URL/test_repo_packed.git \
"$ROOT_PATH"/test_repo_clone_packed &&
- (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo_packed.git &&
+ (cd "$HTTPD_GIT_PATH"/test_repo_packed.git &&
git --bare repack &&
git --bare prune-packed) &&
@@ -105,7 +113,7 @@ test_expect_success 'http-push fetches packed objects' '
git remote rm origin &&
git reflog expire --expire=0 --all &&
git prune &&
- git push -f -v $HTTPD_URL/test_repo_packed.git master)
+ git push -f -v $HTTPD_GIT_URL/test_repo_packed.git master)
'
test_expect_success 'create and delete remote branch' '
@@ -122,6 +130,7 @@ test_expect_success 'create and delete remote branch' '
test_must_fail git show-ref --verify refs/remotes/origin/dev
'
+test -n "$LIB_HTTPD_GIT" ||
test_expect_success 'MKCOL sends directory names with trailing slashes' '
! grep "\"MKCOL.*[^/] HTTP/[^ ]*\"" < "$HTTPD_ROOT_PATH"/access.log
@@ -134,6 +143,7 @@ x5="$x1$x1$x1$x1$x1"
x38="$x5$x5$x5$x5$x5$x5$x5$x1$x1$x1"
x40="$x38$x2"
+test -n "$LIB_HTTPD_GIT" ||
test_expect_success 'PUT and MOVE sends object to URLs with SHA-1 hash suffix' '
sed -e "s/PUT /OP /" -e "s/MOVE /OP /" "$HTTPD_ROOT_PATH"/access.log |
grep -e "\"OP .*/objects/$x2/${x38}_$x40 HTTP/[.0-9]*\" 20[0-9] "
--
1.6.5.35.ge41a3
^ permalink raw reply related
* [PATCH 2/3] remote-helpers: return successfully if everything up-to-date
From: Clemens Buchacher @ 2009-10-25 12:06 UTC (permalink / raw)
To: spearce; +Cc: Mark Lodato, git, Clemens Buchacher
In-Reply-To: <1256472380-924-2-git-send-email-drizzd@aon.at>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
t/t5540-http-push.sh | 2 +-
transport-helper.c | 2 ++
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index 09edd23..2ece661 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -58,7 +58,7 @@ test_expect_success 'push to remote repository with packed refs' '
test $HEAD = $(git rev-parse --verify HEAD))
'
-test_expect_failure 'push already up-to-date' '
+test_expect_success 'push already up-to-date' '
git push
'
diff --git a/transport-helper.c b/transport-helper.c
index 16c6641..5078c71 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -263,6 +263,8 @@ static int push_refs(struct transport *transport,
strbuf_addstr(&buf, ref->name);
strbuf_addch(&buf, '\n');
}
+ if (buf.len == 0)
+ return 0;
transport->verbose = flags & TRANSPORT_PUSH_VERBOSE ? 1 : 0;
standard_options(transport);
--
1.6.5.35.ge41a3
^ permalink raw reply related
* [PATCH 1/3] update http tests according to remote-curl capabilities
From: Clemens Buchacher @ 2009-10-25 12:06 UTC (permalink / raw)
To: spearce; +Cc: Mark Lodato, git, Clemens Buchacher
In-Reply-To: <1256472380-924-1-git-send-email-drizzd@aon.at>
o Pushing packed refs is now fixed.
o The transport helper fails if refs are already up-to-date. Add a
test for that.
o The transport helper will notice if refs are already up-to-date. We
therefore need to update server info in the unpacked-refs test.
o The transport helper will purge deleted branches automatically.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
t/t5540-http-push.sh | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
index f4a2cf6..09edd23 100755
--- a/t/t5540-http-push.sh
+++ b/t/t5540-http-push.sh
@@ -36,6 +36,7 @@ test_expect_success 'setup remote repository' '
cd test_repo.git &&
git --bare update-server-info &&
mv hooks/post-update.sample hooks/post-update &&
+ ORIG_HEAD=$(git rev-parse --verify HEAD) &&
cd - &&
mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
'
@@ -45,7 +46,7 @@ test_expect_success 'clone remote repository' '
git clone $HTTPD_URL/test_repo.git test_repo_clone
'
-test_expect_failure 'push to remote repository with packed refs' '
+test_expect_success 'push to remote repository with packed refs' '
cd "$ROOT_PATH"/test_repo_clone &&
: >path2 &&
git add path2 &&
@@ -57,11 +58,15 @@ test_expect_failure 'push to remote repository with packed refs' '
test $HEAD = $(git rev-parse --verify HEAD))
'
-test_expect_success ' push to remote repository with unpacked refs' '
+test_expect_failure 'push already up-to-date' '
+ git push
+'
+
+test_expect_success 'push to remote repository with unpacked refs' '
(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
rm packed-refs &&
- git update-ref refs/heads/master \
- 0c973ae9bd51902a28466f3850b543fa66a6aaf4) &&
+ git update-ref refs/heads/master $ORIG_HEAD &&
+ git --bare update-server-info) &&
git push &&
(cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
test $HEAD = $(git rev-parse --verify HEAD))
@@ -113,7 +118,6 @@ test_expect_success 'create and delete remote branch' '
git push origin dev &&
git fetch &&
git push origin :dev &&
- git branch -d -r origin/dev &&
git fetch &&
test_must_fail git show-ref --verify refs/remotes/origin/dev
'
--
1.6.5.35.ge41a3
^ permalink raw reply related
* Smart HTTP tests
From: Clemens Buchacher @ 2009-10-25 12:06 UTC (permalink / raw)
To: spearce; +Cc: Mark Lodato, git
Hi Shawn,
This is on top of 1e0eb0f0 (Smart HTTP fetch: gzip requests) in pu.
I had to use the SetHandler syntax as suggested by Mark. My apache setup
(debian 2.2.9-10+lenny) does not resolve the suffix to git-http-backend
relative to document root automatically. Instead it will look for repos in
${GIT_EXEC_PATH}/git-http-backend/git/...
Clemens
^ permalink raw reply
* date change of commit?
From: Alex K @ 2009-10-25 11:35 UTC (permalink / raw)
To: git
Hello,
Is it possible to change the date of a commit?
Thank you,
Alex
^ permalink raw reply
* Re: [PATCH v2 0/2] user-manual: new "getting started" section
From: Jonathan Nieder @ 2009-10-25 11:14 UTC (permalink / raw)
To: Felipe Contreras
Cc: Junio C Hamano, git, Michael J Gruber, J. Bruce Fields,
Hannu Koivisto, Jeff King, Wincent Colaiuta, Matthias Lederhofer
In-Reply-To: <94a0d4530910250243k4cbc3c18l5e018a05e5afdb2d@mail.gmail.com>
Hi Felipe,
Felipe Contreras wrote:
> I disagree. I think it's awfully useful to have color.ui = auto
> configured before reading the multitude of 'git branch', 'git show',
> and 'git diff' commands that are presented on first two sections. So
> useful, in fact, that I think it should be enabled by default.
This is why I think you had a good idea here. When a program doesn’t
behave as I would like, it can be very comforting to know where its
dotfile is and be able to edit it (I’m not sure how I learn this for
most programs). And it is much easier to parse git output without
reading it all when color is turned on, so that is a setting I imagine
could be useful to people.
On the other hand, it is far more pleasant to use a program that
doesn’t need configuration at all. And as I mentioned before, it is
best to avoid wasted time at the beginning of the manual.
> Supposing that color.ui is 'auto' by default,
Should it be? I think it would not be too hard to detect a color
terminal by checking $TERM. Are many people bothered by color? Do we
need some way to make it more obvious how to turn color _off_?
> No, but "improving" needs "changing", and the discussion I see is
> biased towards "not changing".
[...]
> I don't think the user manual is achieving that purpose. I don't know
> if it's the user manual's fault, or git's UI. Both areas need a lot of
> improvement (as the git user survey suggests), and I've tried to
> improve both with a lot resistance in both. So I'm not very hopeful
> anymore.
I hope you have not misunderstood. I cannot speak for everyone else
here, but I know I am happier when (1) fixes match problems to be
solved in a documented way and (2) fixes do not unnecessarily break
unrelated habits. One way to bring this about is to justify each
change by explaining what real problem it will solve and how it avoids
collateral damage. Without that justification, a change is indeed
dangerous and might be worth resisting until it gets clarified. But
this is not meant to prevent fixes from occuring at all.
Could you list some UI patches that were overlooked or not properly
addressed? Maybe people just forgot about them or were waiting for an
updated version, or maybe the problems some solve weren’t articulated
clearly yet. I would be glad to help out in any way I can.
> However, I haven't met any proficient git user that got to that point
> by reading the user manual, so I think it must be completely
> re-thought.
I have met one. (Well, he read the git tutorial and learned by using
git, too.) I think the user manual’s pretty well written, though it
certainly has its gaps and rough spots.
> Judging from the luck I've had pushing even the simplest
> changes I don't think it will improve much more, unfortunately.
Even the simplest changes can be hard. But I hope they do not amount
to nothing. I hope at the very least the git-config manual page will
improve...
Thank you for working on this. I hope you succeed in improving git’s
usability, one way or another.
Regards,
Jonathan
^ permalink raw reply
* Re: [PATCH v2 0/2] user-manual: new "getting started" section
From: Felipe Contreras @ 2009-10-25 9:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Michael J Gruber, Jonathan Nieder, J. Bruce Fields,
Hannu Koivisto, Jeff King, Wincent Colaiuta, Matthias Lederhofer
In-Reply-To: <7vy6n02mrk.fsf@alter.siamese.dyndns.org>
On Sun, Oct 25, 2009 at 6:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> I'm inclined to to discard the first patch.
>>
>> And you decided to mention that after many people including you, have
>> agreed that it's a good idea?
>
> This line of argument is wrong and counterproductive. Of course, after
> reading what others said and thinking about it more myself, I can change
> my mind based on their opinions. Otherwise there is no point in having
> any mailing list discussion.
>
> People propose changes, and two things can happen:
>
> (1) I and others may think it is not a good idea, clarifying argument may
> come from the original author and/or additional arguments defending
> the change may come from others. People who thought it was not a
> good idea may change their mind, and the patch gets accepted. git
> becomes better.
>
> If people cannot change their mind, it is useless to make supporting
> arguments to nudge them to reconsider.
>
> (2) I and others may think it is a good idea, a counterargument comes,
> and people who originally thought it was a good idea may change their
> mind, and the patch does not go in. git is saved from becoming
> worse.
>
> If people cannot change their mind, it is useless to make counter-
> arguments to nudge them to reconsider.
Yes, people can change their mind, but if they do so, they must tell
it as it is, and you didn't:
I think a "Getting started" section that only covers "git config" looks
way out of place in the beginning of this document.
If you would have said it properly "I've changed my mind" there's an
obvious question that arises: Why? What made you change your mind. You
didn't say that either.
> Yes, I originally thought a "getting started" section may be a good idea.
> There is no need to point it out to me.
One never knows =/
> But after I saw that the original author said "_if_ we have to do this,
> keep it short", the comment made me question my previous assumption one
> more time: is it really a good idea to add "getting started", and is it a
> good idea to cover the config command in that section?
>
> After re-reading the first thousand lines of the user manual, I realized
> that the explanation was carefully laid out so that you do not have to be
> taught "git config" in the beginning to be able to follow it. Now, after
> applying your latest patch, if we do not have to teach "config" there,
> what else is left in the section? --- Nothing.
>
> What conclusion do you expect me to reach after such a consideration,
> other than "then let's not have it"?
Aha! That's the explanation I was looking for, and what I think you
should have said in the first place. Now we can do some productive
discussion.
I disagree. I think it's awfully useful to have color.ui = auto
configured before reading the multitude of 'git branch', 'git show',
and 'git diff' commands that are presented on first two sections. So
useful, in fact, that I think it should be enabled by default.
Supposing that color.ui is 'auto' by default, then yeah, I don't see
the point of such a "git config" section so early in the document, but
it must be explained somewhere.
>> If you read the results of the last git survey you'll see that the
>> area that needs most improvement is the documentation.
>
> Yes, I did read it, but what about it? You already know we both want to
> have a good set of documentation.
>
> Remember that "changing" and "improving" is different; some changes may
> not necessarily be improvements. "It needs improving, so let's change it"
> is not an argument. This isn't obviously limited to the documentation but
> also applies to UI changes.
No, but "improving" needs "changing", and the discussion I see is
biased towards "not changing".
>> Also I still
>> see many people doing commits without configuring the user name and
>> email properly and so I've tried very hard to improve the user manual
>> to make it easier for them to understand they must do that.
>
> The "unconfigured user.name is wrong" is the least of the problems for
> people who start commiting without understanding the basic principles.
> People may ask "how do I publish my changes", "how do I discard the
> commit" and "how do I modify the commit two days ago", and teaching them
> things like "reset HEAD^" and "rebase -i", without making them aware of
> the implications will do disservice to them in the long run. That kind of
> self-teaching is already done by people (and for doing so sometimes they
> hurt themselves) by diving into man pages of individual commands before
> understanding the distributedness and its implications, and my hope has
> always been to keep the user-manual a document that teaches things in one
> coherent and hopefully the most useful order.
I don't think the user manual is achieving that purpose. I don't know
if it's the user manual's fault, or git's UI. Both areas need a lot of
improvement (as the git user survey suggests), and I've tried to
improve both with a lot resistance in both. So I'm not very hopeful
anymore.
> The early part of the manual (the first thousand lines) does not talk
> about making commits but lays out the groundwork for a good reason. And
> in order to follow the current structure of the manual, you do not need to
> be taught "config" as the first thing.
>
> It is a totally different story if we are going to rewrite the manual in
> such a way that we start from "hello world". I am not necessarily saying
> it is a bad way to teach [*1*].
>
> But the current "starting from a sightseer, while learning the basic
> concepts like reachability and stuff, and then learn to build on top of
> others' work" structure would also be a valid way to teach, and in that
> presentation order, I do not think teaching "config" sits well at the
> beginning.
IMO the vast majority of git users will use it to just fetch a
repository, and browse through it, and that's because most people are
not developers. Even developers most probably will start that way, and
only start committing after a while.
However, I haven't met any proficient git user that got to that point
by reading the user manual, so I think it must be completely
re-thought. Judging from the luck I've had pushing even the simplest
changes I don't think it will improve much more, unfortunately.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] Use 'fast-forward' all over the place
From: Felipe Contreras @ 2009-10-25 9:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael J Gruber, Shawn O. Pearce
In-Reply-To: <7vtyxoymfa.fsf@alter.siamese.dyndns.org>
On Sun, Oct 25, 2009 at 10:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I'll queue your patch with the following interdiff for now.
>
> * user-manual.txt: do not change emphasis (typography) in this patch---if
> it needs to be changed, that should be a separate patch.
>
> * builtin-fetch.c: an instance of "non fast-forward" was left
> unconverted; fix it.
>
> * contrib/hooks/post-receive-email: likewise.
>
> By the way, in Documentation/git-init.txt there also is another "non
> fast-forward"; since it is not a fix-up to this patch, I left it as-is.
> We may want to change it as well while we are at it.
>
> * git-gui: this is a straight revert of your patch, so that Shawn can
> apply an undated version of this patch to his tree first, and then I
> can pull the result from him.
>
> $ git grep -i -n -e 'fast forward' -- git-gui/
>
> will show what need to be changed. This is a key string of "mc", so
> msgid in *.po files and git-gui.pot file need to consistently change,
> even though the translated strings in *.po files may stay as before.
>
> If you spot misconversions in here (or other "fast forward" that need to be
> converted but I missed), please holler.
It's fine by me :)
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] Use 'fast-forward' all over the place
From: Felipe Contreras @ 2009-10-25 9:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael J Gruber
In-Reply-To: <7v1vks2l8q.fsf@alter.siamese.dyndns.org>
On Sun, Oct 25, 2009 at 6:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> This is the latter half of my review; please see
>
> From: Junio C Hamano <gitster@pobox.com>
> Subject: Re: [PATCH] Use 'fast-forward' all over the place
> Date: Sat, 24 Oct 2009 10:50:05 -0700
> Message-ID: <7v3a587kc2.fsf@alter.siamese.dyndns.org>
>
> for the other half.
>
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>> diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
>> @@ -315,8 +315,8 @@ generate_update_branch_email()
>> # "remotes/" will be ignored as well.
>>
>> # List all of the revisions that were removed by this update, in a
>> - # fast forward update, this list will be empty, because rev-list O
>> - # ^N is empty. For a non fast forward, O ^N is the list of removed
>> + # fast-forward update, this list will be empty, because rev-list O
>> + # ^N is empty. For a non fast-forward, O ^N is the list of removed
>
> Wasn't non-fast-forward a single compound word of three?
Yes, but as I mentioned before; it's not something I'm tackling on
this patch: only "non-fast forward". I think a separate patch should
be done for "non fast.forward" -> non-fast-forward.
>> diff --git a/git-gui/lib/branch_create.tcl b/git-gui/lib/branch_create.tcl
>> index 3817771..f1235c7 100644
>> --- a/git-gui/lib/branch_create.tcl
>> +++ b/git-gui/lib/branch_create.tcl
>> @@ -77,7 +77,7 @@ constructor dialog {} {
>> -variable @opt_merge
>> pack $w.options.merge.no -side left
>> radiobutton $w.options.merge.ff \
>> - -text [mc "Fast Forward Only"] \
>> + -text [mc "Fast-forward Only"] \
>> -value ff \
>> -variable @opt_merge
>> pack $w.options.merge.ff -side left
>
> Please leave git-gui out; (1) it is not managed by me and the patch should
> be fed to Shawn separately, and (2) updating [mc] keystrings must need
> matching updates to the translation file and the templates.
>
> I also think this is a label string and the capitalization must be kept,
> i.e. "Fast-Forward Only".
Ok.
>> diff --git a/t/t1001-read-tree-m-2way.sh b/t/t1001-read-tree-m-2way.sh
>> @@ -51,7 +51,7 @@ check_cache_at () {
>> }
>>
>> cat >bozbar-old <<\EOF
>> -This is a sample file used in two-way fast forward merge
>> +This is a sample file used in two-way fast-forward merge
>> tests. Its second line ends with a magic word bozbar
>> which will be modified by the merged head to gnusto.
>> It has some extra lines so that external tools can
>
> Doesn't changing this change the actual test blob used? Do you know if
> the test still passes when git is not broken?
I didn't check. I will.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] Use 'fast-forward' all over the place
From: Felipe Contreras @ 2009-10-25 8:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael J Gruber
In-Reply-To: <7v3a5860gr.fsf@alter.siamese.dyndns.org>
On Sat, Oct 24, 2009 at 10:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>>> I suspect the patch would have been much easier to the reviewers it it
>>> stated somewhere in the log message:
>>>
>>> (1) how the mechanical change was produced;
>>
>> There wasn't such.
>
> That is actually a bad news; it is even worse than mechanical spotting
> followed by manual inspection. It would force us feel _more_ worried, as
> we would then need to grep for leftovers that your manual conversion may
> not have caught. Sigh...
Perhaps next time I'll do one round manual, and another one automatic,
but in this case I didn't think it was so difficult to review hunk by
hunk.
>>> (2) what criteria was used to choose between leaving the mechanical
>>> change as-is and rewording them manually; and
>>
>> If it wasn't straight forward. I considered the following straightforward:
>> fast forward -> fast-forward
>> fast forwarded -> fast-forwarded
>> fast forwarding -> fast-forwarding
>> fast forwardable -> fast-forwardable
>> non-fast forward -> non-fast-forward
>> Fast forward -> Fast-forward
>> Fast forwarding -> Fast-forwarding
>
> All of these are what "s/([fF])ast forward/$1ast-forward/g" does, aren't
> they?
I guess, yes. But that's not how I did it, so I couldn't be sure.
>> I couldn't parse that. From what I can see "Fast forward" was
>> emphasized because the author thought the words didn't make much sense
>> separated. Now that the word is fast-forward, there's no need to
>> emphasize.
>
> Even after your patch, hunk beginning on line 1384 of the
> user-manual says
>
> ... then git just performs a "fast-forward"; the head of the ...
>
> and I think you did the right thing by keeping these dq here. This is the
> first occurrence of the word followed by its explanation and for that
> reason, the word deserves to be emphasized---IOW, the context calls for an
> emphasis.
Yes, exactly.
> In the "Important note!" part, we talk about the pull operation that
> normally creates a merge commit, and _in contrast_, under a particular
> condition (namely, "no local changes"), it does something different
> (namely, a "fast-forward"). We should keep the emphasis on "fast-forward"
> here for exactly the same reason---the context calls for an emphasis
I don't think so. The emphasis in this case breaks the readability of
the text for no reason:
with no local changes git will simply do a fast-forward merge
Can be perfectly understood as it is. But in any case, that's out of
the scope of this patch.
Cheers.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH] Use 'fast-forward' all over the place
From: Junio C Hamano @ 2009-10-25 7:22 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Michael J Gruber, Shawn O. Pearce
In-Reply-To: <7v4opoym7w.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I'll queue your patch with the following interdiff for now.
>
> Sorry, I think I sent a wrong interdiff.
Nevermind; the one I sent out was the right one after all.
^ 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