* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Torsten Bögershausen @ 2013-01-02 0:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Torsten Bögershausen, git
In-Reply-To: <7v7gnw8slq.fsf@alter.siamese.dyndns.org>
On 01.01.13 23:07, Junio C Hamano wrote:
[snip]
> What it checks looks like a good start, but the indentation of it
> (and the log message) seems very screwed up.
OK
> I also have to wonder what's the false positive rate of this. When
> you are preparing a new test, you would ideally want a mode that
> checks only parts that you just added, without seeing noises from
> existing violations and false positives from the part you did not
> touch. Otherwise, it will be too cumbersome to run for developers,
> and the check mechanism will end up used by nobody.
>
The script found all problems which make the testsuite (unecessary) fail on Mac OS X.
The false positive rate is currently 0% (otherwise I should not have send it to the list)
The suggestion is to run it every time the test suite is run, at the begining.
And it seems to be fast enough:
$ time ./check-non-portable-shell.pl ../../git.master/t/t[0-9]*.sh
real 0m0.263s
user 0m0.239s
sys 0m0.021s
/Torsten
^ permalink raw reply
* Re: [RFC] pack-objects: compression level for non-blobs
From: Junio C Hamano @ 2013-01-01 23:47 UTC (permalink / raw)
To: Shawn Pearce; +Cc: Duy Nguyen, Jeff King, David Michael Barr, Git Mailing List
In-Reply-To: <CAJo=hJsZedd0kfYJnXPhcud8bz3mgU0NMf6O6-_PY1yqv-EfDg@mail.gmail.com>
Shawn Pearce <spearce@spearce.org> writes:
> How blobs are written is very different, Junio's
> implementation is strictly better than JGit's[1].
I do not think there can be a single ordering that is strictly
better than any other one. The "clump all objects in a delta family
and write them width-first, starting from the base object" may give
you a reasonable trade-off for a result of normal repack, but if you
repack (like I do) with very shallow --depth with wide --window to
really get a tight pack, a delta may end up having too may uncles
between it and its father, requiring a large seek to skip over all
the uncles in order to grab the delta data, after you reconstitute
the delta base object.
^ permalink raw reply
* [PATCH 3/3] SubmittingPatches: remove overlong checklist
From: Junio C Hamano @ 2013-01-01 23:24 UTC (permalink / raw)
To: git; +Cc: Jason Holden
In-Reply-To: <1357082695-29713-1-git-send-email-gitster@pobox.com>
The section is no longer a concise checklist. It also talks about
things that are not covered in the "Long version" text, which means
people need to read both, covering more or less the same thing in
different phrasing.
Fold the details into the main text and remove the section.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/SubmittingPatches | 126 +++++++++++++++++-----------------------
1 file changed, 53 insertions(+), 73 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index e810263..a7daad3 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -1,65 +1,3 @@
-Checklist (and a short version for the impatient):
-
- Commits:
-
- - make commits of logical units
- - check for unnecessary whitespace with "git diff --check"
- before committing
- - do not check in commented out code or unneeded files
- - the first line of the commit message should be a short
- description (50 characters is the soft limit, see DISCUSSION
- in git-commit(1)), and should skip the full stop
- - it is also conventional in most cases to prefix the
- first line with "area: " where the area is a filename
- or identifier for the general area of the code being
- modified, e.g.
- . archive: ustar header checksum is computed unsigned
- . git-cherry-pick.txt: clarify the use of revision range notation
- (if in doubt which identifier to use, run "git log --no-merges"
- on the files you are modifying to see the current conventions)
- - the body should provide a meaningful commit message, which:
- . explains the problem the change tries to solve, iow, what
- is wrong with the current code without the change.
- . justifies the way the change solves the problem, iow, why
- the result with the change is better.
- . alternate solutions considered but discarded, if any.
- - describe changes in imperative mood, e.g. "make xyzzy do frotz"
- instead of "[This patch] makes xyzzy do frotz" or "[I] changed
- xyzzy to do frotz", as if you are giving orders to the codebase
- to change its behaviour.
- - try to make sure your explanation can be understood without
- external resources. Instead of giving a URL to a mailing list
- archive, summarize the relevant points of the discussion.
- - add a "Signed-off-by: Your Name <you@example.com>" line to the
- commit message (or just use the option "-s" when committing)
- to confirm that you agree to the Developer's Certificate of Origin
- - make sure that you have tests for the bug you are fixing
- - make sure that the test suite passes after your commit
-
- Patch:
-
- - use "git format-patch -M" to create the patch
- - do not PGP sign your patch
- - do not attach your patch, but read in the mail
- body, unless you cannot teach your mailer to
- leave the formatting of the patch alone.
- - be careful doing cut & paste into your mailer, not to
- corrupt whitespaces.
- - provide additional information (which is unsuitable for
- the commit message) between the "---" and the diffstat
- - if you change, add, or remove a command line option or
- make some other user interface change, the associated
- documentation should be updated as well.
- - if your name is not writable in ASCII, make sure that
- you send off a message in the correct encoding.
- - send the patch to the list (git@vger.kernel.org) and the
- maintainer (gitster@pobox.com) if (and only if) the patch
- is ready for inclusion. If you use git-send-email(1),
- please test it first by sending email to yourself.
- - see below for instructions specific to your mailer
-
-Long version:
-
Here are some guidelines for people who want to contribute their code
to this software.
@@ -119,13 +57,52 @@ change, the approach taken by the change, and if relevant how this
differs substantially from the prior version, are all good things
to have.
+Make sure that you have tests for the bug you are fixing.
+
+When adding a new feature, make sure that you have new tests to show
+the feature triggers the new behaviour when it should, and to show the
+feature does not trigger when it shouldn't. Also make sure that the
+test suite passes after your commit. Do not forget to update the
+documentation to describe the updated behaviour.
+
Oh, another thing. I am picky about whitespaces. Make sure your
changes do not trigger errors with the sample pre-commit hook shipped
in templates/hooks--pre-commit. To help ensure this does not happen,
run git diff --check on your changes before you commit.
+(2) Describe your changes well.
+
+The first line of the commit message should be a short description (50
+characters is the soft limit, see DISCUSSION in git-commit(1)), and
+should skip the full stop. It is also conventional in most cases to
+prefix the first line with "area: " where the area is a filename or
+identifier for the general area of the code being modified, e.g.
+
+ . archive: ustar header checksum is computed unsigned
+ . git-cherry-pick.txt: clarify the use of revision range notation
+
+If in doubt which identifier to use, run "git log --no-merges" on the
+files you are modifying to see the current conventions.
+
+The body should provide a meaningful commit message, which:
-(2) Generate your patch using git tools out of your commits.
+ . explains the problem the change tries to solve, iow, what is wrong
+ with the current code without the change.
+
+ . justifies the way the change solves the problem, iow, why the
+ result with the change is better.
+
+ . alternate solutions considered but discarded, if any.
+
+Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
+instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
+to do frotz", as if you are giving orders to the codebase to change
+its behaviour. Try to make sure your explanation can be understood
+without external resources. Instead of giving a URL to a mailing list
+archive, summarize the relevant points of the discussion.
+
+
+(3) Generate your patch using git tools out of your commits.
git based diff tools generate unidiff which is the preferred format.
@@ -133,22 +110,27 @@ You do not have to be afraid to use -M option to "git diff" or
"git format-patch", if your patch involves file renames. The
receiving end can handle them just fine.
-Please make sure your patch does not include any extra files
-which do not belong in a patch submission. Make sure to review
+Please make sure your patch does not add commented out debugging code,
+or include any extra files which do not relate to what your patch
+is trying to achieve. Make sure to review
your patch after generating it, to ensure accuracy. Before
sending out, please make sure it cleanly applies to the "master"
branch head. If you are preparing a work based on "next" branch,
that is fine, but please mark it as such.
-(3) Sending your patches.
+(4) Sending your patches.
People on the git mailing list need to be able to read and
comment on the changes you are submitting. It is important for
a developer to be able to "quote" your changes, using standard
e-mail tools, so that they may comment on specific portions of
your code. For this reason, all patches should be submitted
-"inline". WARNING: Be wary of your MUAs word-wrap
+"inline". If your log message (including your name on the
+Signed-off-by line) is not writable in ASCII, make sure that
+you send off a message in the correct encoding.
+
+WARNING: Be wary of your MUAs word-wrap
corrupting your patch. Do not cut-n-paste your patch; you can
lose tabs that way if you are not careful.
@@ -200,16 +182,14 @@ patch, format it as "multipart/signed", not a text/plain message
that starts with '-----BEGIN PGP SIGNED MESSAGE-----'. That is
not a text/plain, it's something else.
-Unless your patch is a very trivial and an obviously correct one,
-first send it with "To:" set to the mailing list, with "cc:" listing
+Send your patch with "To:" set to the mailing list, with "cc:" listing
people who are involved in the area you are touching (the output from
"git blame $path" and "git shortlog --no-merges $path" would help to
identify them), to solicit comments and reviews. After the list
reached a consensus that it is a good idea to apply the patch, re-send
-it with "To:" set to the maintainer and optionally "cc:" the list for
-inclusion. Do not forget to add trailers such as "Acked-by:",
-"Reviewed-by:" and "Tested-by:" after your "Signed-off-by:" line as
-necessary.
+it with "To:" set to the maintainer and "cc:" the list for inclusion.
+Do not forget to add trailers such as "Acked-by:", "Reviewed-by:" and
+"Tested-by:" after your "Signed-off-by:" line as necessary.
(4) Sign your work
--
1.8.1.209.gc32ab23
^ permalink raw reply related
* [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories
From: Junio C Hamano @ 2013-01-01 23:24 UTC (permalink / raw)
To: git; +Cc: Jason Holden
In-Reply-To: <1357082695-29713-1-git-send-email-gitster@pobox.com>
These were only mentioned in periodical "A note from the maintainer"
posting and not in the documentation suite. SubmittingPatches has a
section to help contributors decide on what commit to base their
changes, which is the most suitable place for this information.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/SubmittingPatches | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index b9fd182..e810263 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -88,6 +88,10 @@ change is relevant to.
wait until some of the dependent topics graduate to 'master', and
rebase your work.
+ - Some parts of the system have dedicated maintainers with their own
+ repositories (see the section "Subsystems" below). Changes to
+ these parts should be based on their trees.
+
To find the tip of a topic branch, run "git log --first-parent
master..pu" and look for the merge commit. The second parent of this
commit is the tip of the topic branch.
@@ -279,6 +283,26 @@ You can also create your own tag or use one that's in common usage
such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
------------------------------------------------
+Subsystems with dedicated maintainers
+
+Some parts of the system have dedicated maintainers with their own
+repositories.
+
+ - git-gui/ comes from git-gui project, maintained by Pat Thoyts:
+
+ git://repo.or.cz/git-gui.git
+
+ - gitk-git/ comes from Paul Mackerras's gitk project:
+
+ git://ozlabs.org/~paulus/gitk
+
+ - po/ comes from the localization coordinator, Jiang Xin:
+
+ https://github.com/git-l10n/git-po/
+
+Patches to these parts should be based on their trees.
+
+------------------------------------------------
An ideal patch flow
Here is an ideal patch flow for this project the current maintainer
--
1.8.1.209.gc32ab23
^ permalink raw reply related
* [PATCH 1/3] SubmittingPatches: who am I and who cares?
From: Junio C Hamano @ 2013-01-01 23:24 UTC (permalink / raw)
To: git; +Cc: Jason Holden
In-Reply-To: <1357082695-29713-1-git-send-email-gitster@pobox.com>
The introductory text in the "long version" talks about the origin
of this document with "I started ...", but it is unclear who that I
is, and more importantly, it is not interesting how it was started.
Just state the purpose of the document to help readers decide if it
is releavant to them.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Documentation/SubmittingPatches | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index c34c9d1..b9fd182 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -60,14 +60,8 @@ Checklist (and a short version for the impatient):
Long version:
-I started reading over the SubmittingPatches document for Linux
-kernel, primarily because I wanted to have a document similar to
-it for the core GIT to make sure people understand what they are
-doing when they write "Signed-off-by" line.
-
-But the patch submission requirements are a lot more relaxed
-here on the technical/contents front, because the core GIT is
-thousand times smaller ;-). So here is only the relevant bits.
+Here are some guidelines for people who want to contribute their code
+to this software.
(0) Decide what to base your work on.
--
1.8.1.209.gc32ab23
^ permalink raw reply related
* [PATCH 0/3] Update SubmittingPatches
From: Junio C Hamano @ 2013-01-01 23:24 UTC (permalink / raw)
To: git; +Cc: Jason Holden
The main thing this series wants to do is the second one, but I
wanted to reduce the clutter in this document while at it.
Junio C Hamano (3):
SubmittingPatches: who am I and who cares?
SubmittingPatches: mention subsystems with dedicated repositories
SubmittingPatches: remove overlong checklist
Documentation/SubmittingPatches | 160 ++++++++++++++++++++--------------------
1 file changed, 79 insertions(+), 81 deletions(-)
--
1.8.1.209.gc32ab23
^ permalink raw reply
* [PATCH V2] t9810: Do not use sed -i
From: Torsten Bögershausen @ 2013-01-01 23:20 UTC (permalink / raw)
To: git, pw; +Cc: tboegi
sed -i is not portable on all systems.
Use sed with different input and output files.
Utilize a tmp file whenever needed
Added missing && at 2 places
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
Changes since v1:
Use "sed <infile >outfile"
Added missing && at 2 places (thanks for catching)
t/t9810-git-p4-rcs.sh | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
index 0c2fc3e..01a9921 100755
--- a/t/t9810-git-p4-rcs.sh
+++ b/t/t9810-git-p4-rcs.sh
@@ -26,10 +26,8 @@ test_expect_success 'init depot' '
line7
line8
EOF
- cp filek fileko &&
- sed -i "s/Revision/Revision: do not scrub me/" fileko
- cp fileko file_text &&
- sed -i "s/Id/Id: do not scrub me/" file_text
+ sed "s/Revision/Revision: do not scrub me/" <filek >fileko &&
+ sed "s/Id/Id: do not scrub me/" <fileko >file_text &&
p4 add -t text+k filek &&
p4 submit -d "filek" &&
p4 add -t text+ko fileko &&
@@ -88,7 +86,8 @@ test_expect_success 'edit far away from RCS lines' '
(
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
- sed -i "s/^line7/line7 edit/" filek &&
+ sed "s/^line7/line7 edit/" <filek >filek.tmp &&
+ mv -f filek.tmp filek &&
git commit -m "filek line7 edit" filek &&
git p4 submit &&
scrub_k_check filek
@@ -105,7 +104,8 @@ test_expect_success 'edit near RCS lines' '
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
git config git-p4.attemptRCSCleanup true &&
- sed -i "s/^line4/line4 edit/" filek &&
+ sed "s/^line4/line4 edit/" <filek >filek.tmp &&
+ mv -f filek.tmp filek &&
git commit -m "filek line4 edit" filek &&
git p4 submit &&
scrub_k_check filek
@@ -122,7 +122,8 @@ test_expect_success 'edit keyword lines' '
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
git config git-p4.attemptRCSCleanup true &&
- sed -i "/Revision/d" filek &&
+ sed "/Revision/d" <filek >filek.tmp &&
+ mv -f filek.tmp filek &&
git commit -m "filek remove Revision line" filek &&
git p4 submit &&
scrub_k_check filek
@@ -139,7 +140,8 @@ test_expect_success 'scrub ko files differently' '
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
git config git-p4.attemptRCSCleanup true &&
- sed -i "s/^line4/line4 edit/" fileko &&
+ sed <fileko "s/^line4/line4 edit/" >fileko.tmp &&
+ mv -f fileko.tmp fileko &&
git commit -m "fileko line4 edit" fileko &&
git p4 submit &&
scrub_ko_check fileko &&
@@ -189,12 +191,14 @@ test_expect_success 'do not scrub plain text' '
cd "$git" &&
git config git-p4.skipSubmitEdit true &&
git config git-p4.attemptRCSCleanup true &&
- sed -i "s/^line4/line4 edit/" file_text &&
+ sed "s/^line4/line4 edit/" <file_text >file_text.tmp &&
+ mv -f file_text.tmp file_text &&
git commit -m "file_text line4 edit" file_text &&
(
cd "$cli" &&
p4 open file_text &&
- sed -i "s/^line5/line5 p4 edit/" file_text &&
+ sed "s/^line5/line5 p4 edit/" <file_text >file_text.tmp &&
+ mv -f file_text.tmp file_text &&
p4 submit -d "file5 p4 edit"
) &&
echo s | test_expect_code 1 git p4 submit &&
--
1.8.0.197.g5a90748
^ permalink raw reply related
* Re: [PATCH 6/8] Make the Manual Directory if Needed
From: greened @ 2013-01-01 22:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jesper L. Nielsen
In-Reply-To: <7vobh88tp3.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile
>> index 36ae3e4..52d6fb9 100644
>> --- a/contrib/subtree/Makefile
>> +++ b/contrib/subtree/Makefile
>> @@ -35,6 +35,7 @@ install: $(GIT_SUBTREE)
>> install-doc: install-man
>>
>> install-man: $(GIT_SUBTREE_DOC)
>> + mkdir -p $(man1dir)
>
> We seem to use "$(INSTALL) -d -m 755" for this kind of thing (see
> the Documentation/Makefile).
Ok, will fix.
-Dave
^ permalink raw reply
* Re: [PATCH 5/8] Honor DESTDIR
From: greened @ 2013-01-01 22:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Adam Tkac
In-Reply-To: <7vsj6k8tr8.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> "David A. Greene" <greened@obbligato.org> writes:
>
>> From: Adam Tkac <atkac@redhat.com>
>>
>> Teach git-subtree's Makefile to honor DESTDIR.
>>
>> Author: Adam Tkac <atkac@redhat.com>
>>
>> Signed-off-by: Adam Tkac <atkac@redhat.com>
>>
>> Signed-off-by: David A. Greene <greened@obbligato.org>
>> ---
>
> The contents of the patch looks sensible; the above is questionable
> as all the other messages in this series, though. Did any of our
> tools cause this failure? If so I would like to know more about it.
What failure are you referring to? When I used git send-email --author,
the Author: line was commented out. I assumed I was supposed to
uncomment it. Guess I was wrong.
I'll re-send the series since you pointed out a number of improvements.
-David
^ permalink raw reply
* Re: [PATCH 4/8] Fix Synopsis
From: greened @ 2013-01-01 22:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwqvw8tur.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> -git subtree add --prefix=<prefix> <commit>
>> +git subtree add --prefix=<prefix> <refspec>
>
> Again, this is not <refspec> but <commit>.
Ok, I need to study the terminology. :)
>> +git subtree add --prefix=<prefix> <repository> <refspec>
>
> This is given to "fetch" and it seems to acccept any <refspec>, so
> it is probably a good change (I didn't fully follow the codepath,
> though).
I think you are correct.
-David
^ permalink raw reply
* Re: [PATCH 3/8] Better Error Handling for add
From: greened @ 2013-01-01 22:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v1ue4a8i0.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
>> index 7ceb413..b8a807a 100755
>> --- a/contrib/subtree/git-subtree.sh
>> +++ b/contrib/subtree/git-subtree.sh
>> @@ -509,8 +509,20 @@ cmd_add()
>> ensure_clean
>>
>> if [ $# -eq 1 ]; then
>> + ref=$(git check-ref-format --normalize "refs/heads/$1") ||
>> + die "'$1' is not a valid refspec. Are you missing a branch?"
>
> Is a user forbidden from passing a commit that is not at the tip of
> an existing branch? In other words, is
>
> $ subtree add origin/next~4^2
>
> forbidden?
Good point. It probably shouldn't be. I think rev-parse should be
enough of a check.
>> + rev=$(git rev-parse --verify $1) ||
>> + die "'$1' is not a valid refspec. Are you missing a branch?"
>> +
>> "cmd_add_commit" "$@"
>
> If you want to make sure you give a comit to add_commit, you can
> probably say something like this:
>
> git rev-parse -q --verify "$1^{commit}" >/dev/null ||
> die "'$1' does not refer to a commit"
What does $1^{commit} mean? I think your suggestion is what I want but
I don't know what it means yet. :)
-David
^ permalink raw reply
* Re: [PATCH 2/8] Add --unannotate
From: greened @ 2013-01-01 22:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, James Nylen
In-Reply-To: <7v623ga8vs.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> "David A. Greene" <greened@obbligato.org> writes:
>
>> From: James Nylen <jnylen@gmail.com>
>>
>> Teach git-subtree about --unannotate. This option strips a prefix
>> from a commit message when doing a subtree split.
>
> Hrm. This looks like a workaround for a short-sighted misdesign of
> the annotate option that only allowed prefixing a fixed string. I
> have to wonder if it is better to deprecate --annotate and replace
> it with a more general "commit log rewriting" facility that can
> cover both use cases?
That's not a bad idea. I'd have to think a bit about a sensible design.
Do you have any ideas, James?
In the meantime, will you apply the patch or do you prefer a new design?
-David
^ permalink raw reply
* Re: [PATCH 1/8] Use %B for Split Subject/Body
From: greened @ 2013-01-01 22:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Techlive Zheng
In-Reply-To: <7va9ssa94l.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> "David A. Greene" <greened@obbligato.org> writes:
>
>> Subject: Re: [PATCH 1/8] Use %B for Split Subject/Body
>
> This needs to say "contrib/subtree" somewhere (applies to all
> patches in this series).
Ok. Shall I re-send everything?
>> From: Techlive Zheng <techlivezheng@gmail.com>
>>
>> Use %B to format the commit message and body to avoid an extra newline
>> if a commit only has a subject line.
>>
>> Author: Techlive Zheng <techlivezheng@gmail.com>
>
> This needs to be a S-o-b instead; is it a real name, by the way?
Ok. No idea about the name but his online presence seems consistent at
least.
>> +# Save this hash for testing later.
>> +
>> +subdir_hash=`git rev-parse HEAD`
>> +
>
> We prefer $() over ``; much more readable.
Ack, of course. I don't know how I missed that.
>> # 15
>> test_expect_success 'add main6' '
>> create main6 &&
>
> Why?
It was in the original testsuite from Avery. I didn't add or remove any
tests when I first integrated git-subtree.
>> @@ -235,7 +238,19 @@ test_expect_success 'check split with --branch' '
>> check_equal ''"$(git rev-parse splitbr1)"'' "$spl1"
>
> Is quoting screwed up around here (and in many other places in this
> patch)? What are these no-op '' doing?
I assumed they are there to get the double-quotes around the command.
I'll see about removing them.
>> -# 25
>> +#25
>
> Why the lossage of a SP?
I think this got fixed later in the series.
> It may make sense to lose these "# num" that will have to be touched
> every time somebody inserts new test pieces in the middle, as a
> preparatory step before any of these patches, by the way. That will
> reduce noise in the patches for real changes.
Yeah, I know, but it makes it really easy to find a test when something
goes wrong.
-David
^ permalink raw reply
* Re: [PATCH 1/8] Use %B for Split Subject/Body
From: greened @ 2013-01-01 22:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Techlive Zheng
In-Reply-To: <7vpq1pbg1k.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Also, please be careful about the subject line. I doubt that these
> 8 patches will stand out as relating to "contrib/subtree", when mixed
> in 200 line output of "git shortlog --no-merges".
Ah, ok. I'll be more careful next time.
-David
^ permalink raw reply
* Re: [BUG] two-way read-tree can write null sha1s into index
From: Junio C Hamano @ 2013-01-01 22:24 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Nieder, git
In-Reply-To: <20121229205154.GA21058@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> So I think we need to update twoway_merge to recognize unmerged entries,
> which gives us two options:
>
> 1. Reject the merge.
>
> 2. Throw away the current unmerged entry in favor of the "new" entry
> (when old and new are the same, of course; otherwise we would
> reject).
>
> I think (2) is the right thing.
As "--reset" in "read-tree --reset -u A B" is a way to say "we know
we are based on A and we want to go to B no matter what", I agree we
should not reject the merge.
With -m instead of --reset, I am not sure what the right semantics
should be, though.
^ permalink raw reply
* Re: [PATCH 1/8] Use %B for Split Subject/Body
From: greened @ 2013-01-01 22:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Techlive Zheng
In-Reply-To: <7vtxr1bg4g.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> "David A. Greene" <greened@obbligato.org> writes:
>
>> From: Techlive Zheng <techlivezheng@gmail.com>
>>
>> Use %B to format the commit message and body to avoid an extra newline
>> if a commit only has a subject line.
>
> Is this an unconditional improvement, or is it generally an
> improvement but for some users it may be a regression? I am
> guessing it is the former but am just making sure.
The former.
>> Author: Techlive Zheng <techlivezheng@gmail.com>
>>
>> Signed-off-by: David A. Greene <greened@obbligato.org>
>
> Please don't do "Author: " which does not add anything new. That is
> what "From: " is for. Instead it needs to be a sign-off.
Ok. Unfortunately I sent a number of patches like that. Do you want me
to re-send them?
> Also, is that a real name, I have to wonder?
No idea. Not likely, I'd say.
-David
^ permalink raw reply
* Re: [PATCH 2/4] t9810: Do not use sed -i
From: Junio C Hamano @ 2013-01-01 22:12 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git, pw
In-Reply-To: <201301012240.37769.tboegi@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> sed -i is not portable on all systems.
> Use sed with different input and output files.
> Utilize a tmp file whenever needed
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
> t/t9810-git-p4-rcs.sh | 24 ++++++++++++++----------
> 1 file changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/t/t9810-git-p4-rcs.sh b/t/t9810-git-p4-rcs.sh
> index 0c2fc3e..5bf9291 100755
> --- a/t/t9810-git-p4-rcs.sh
> +++ b/t/t9810-git-p4-rcs.sh
> @@ -26,10 +26,8 @@ test_expect_success 'init depot' '
> line7
> line8
> EOF
> - cp filek fileko &&
> - sed -i "s/Revision/Revision: do not scrub me/" fileko
> - cp fileko file_text &&
> - sed -i "s/Id/Id: do not scrub me/" file_text
> + sed <filek "s/Revision/Revision: do not scrub me/" >fileko
> + sed <fileko "s/Id/Id: do not scrub me/" >file_text
Making it shorter and more correct ;-), which is good, but you are
losing the && chaining. Also it is more customary to have
redirection at the end, unless it is to redirect a numbered file
descriptor (e.g. "echo >&2 error message"). I.e.
sed "s/Revision/Revision: do not scrub me/" <filek >fileko &&
sed "s/Id/Id: do not scrub me/" <fileko >file_text
^ permalink raw reply
* Re: [PATCH] Documentation: full-ness of a bundle is significant for cloning
From: W. Trevor King @ 2013-01-01 21:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kirill Brilliantov
In-Reply-To: <7vehi4a9za.fsf_-_@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 146 bytes --]
On Tue, Jan 01, 2013 at 01:07:05PM -0800, Junio C Hamano wrote:
> critical hint to tell readers why thhis particular bundle can be
s/thhis/this/
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] test: Add target test-lint-shell-syntax
From: Junio C Hamano @ 2013-01-01 22:07 UTC (permalink / raw)
To: Torsten Bögershausen; +Cc: git
In-Reply-To: <201301012240.10722.tboegi@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> Add the perl script "check-non-portable-shell.pl" to detect non-portable
> shell syntax
> Many systems use gnu tools which accept an extended syntax in shell scripts,
> which is not portable on all systems and causes the test suite to fail.
>
> To prevent contributors using e.g. Linux to add non-portable test code,
> "check-non-portable-shell.pl" is run as part of
> "make test" or "make in the t/ directory.
>
> "echo -n" is an example of a statement working on Linux,
> but not on e.g. Mac OS X.
>
> Beside "echo -n" we check for
> "sed -i",
> arrays in shell scripts (declare statement),
> "which" (use type instead),
> or "==" (bash style of =)
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
What it checks looks like a good start, but the indentation of it
(and the log message) seems very screwed up.
I also have to wonder what's the false positive rate of this. When
you are preparing a new test, you would ideally want a mode that
checks only parts that you just added, without seeing noises from
existing violations and false positives from the part you did not
touch. Otherwise, it will be too cumbersome to run for developers,
and the check mechanism will end up used by nobody.
> +######################################################################
> +# Test t0000..t9999.sh for non portable shell scripts #
> +# Examples are "echo -n" or "sed -i" #
> +# This script can be called with one or more filenames as parameters #
> +#
> +######################################################################
Just a style thing (style requests are not optional, though ;-), but
these box comments are moderately annoying to read and extremely
annoying to modify. Writing it like this:
> +#
> +# Test t0000..t9999.sh for non portable shell scripts
> +# Examples are "echo -n" or "sed -i"
> +# This script can be called with one or more filenames as parameters
> +#
should be sufficiently loud.
^ permalink raw reply
* Re: [PATCH] Avoid using non-POSIX cp options
From: Junio C Hamano @ 2013-01-01 21:57 UTC (permalink / raw)
To: Ben Walton; +Cc: git
In-Reply-To: <1357071197-7927-1-git-send-email-bdwalton@gmail.com>
Thanks; I think I already have 2d3ac9a (t3600: Avoid "cp -a", which
is a GNUism, 2012-12-18)
^ permalink raw reply
* Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
From: Junio C Hamano @ 2013-01-01 21:54 UTC (permalink / raw)
To: esr; +Cc: git
In-Reply-To: <20130101172645.GA5506@thyrsus.com>
"Eric S. Raymond" <esr@thyrsus.com> writes:
> The combination of git-cvsimport and cvsps had serious problems.
> Among these were:
>
> (1) Analysis of branchy repos was buggy in multiple ways in both
> programs, leading to incorrect repo translations.
>
> (2) Even after a correct branch analysis, extra (redundant) fileops
> would often be generated on the new-branch side.
>
> (3) Inability to report more than one tag pointing to the same revision.
>
> (4) Failure in certain cases of clock-skew reported by the t9603 test.
>
> (5) Failure to use commitids for changeset ordering in cases were this
> would have prevented clock skew from causing incorrect grouping.
>
> Problems 2-5 and portions of problem 1 have been solved by a major
> rewrite of cvsps (the 3.x release series); it now emits a git
> fast-import stream.
So..., is this a flag-day patch?
After this is merged, users who have been interoperating with CVS
repositories with the older cvsps have to install the updated cvsps
before using a new version of Git that ships with it? As long as
they update both cvsps and cvsimport, they can continue using the
existing repository to get updates from the same upstream CVS
repository without losing hisory continuity?
I would have preferred an addition of "git cvsimport-new" (or rename
of the existing one to "git cvsimport-old"), with additional tests
that compare the results of these two implemenations on simple CVS
history that cvsimport-old did *not* screw up, to ensure that (1)
people with existing set-up can choose to keep using the old one,
perhaps by tweaking their process to use cvsimport-old, and (2) the
updated one will give these people the identical conversion results,
as long as the history they have been interacting with do not have
the corner cases that trigger bugs in older cvsps.
Or am I being too conservative?
^ permalink raw reply
* Re: [PATCH 7/8] Ignore git-subtree
From: Junio C Hamano @ 2013-01-01 21:44 UTC (permalink / raw)
To: David A. Greene; +Cc: git, Michael Schubert
In-Reply-To: <1357012655-24974-8-git-send-email-greened@obbligato.org>
"David A. Greene" <greened@obbligato.org> writes:
> From: Michael Schubert <mschub@elegosoft.com>
>
> Add the git-subtree command executable to .gitignore.
>
> Author: Michael Schubert <mschub@elegosoft.com>
>
> Signed-off-by: Michael Schubert <mschub@elegosoft.com>
>
> Signed-off-by: David A. Greene <greened@obbligato.org>
> ---
Seems sensible; I think I have this already in my tree.
> contrib/subtree/.gitignore | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/contrib/subtree/.gitignore b/contrib/subtree/.gitignore
> index 7e77c9d..91360a3 100644
> --- a/contrib/subtree/.gitignore
> +++ b/contrib/subtree/.gitignore
> @@ -1,4 +1,5 @@
> *~
> +git-subtree
> git-subtree.xml
> git-subtree.1
> mainline
^ permalink raw reply
* Re: [PATCH 6/8] Make the Manual Directory if Needed
From: Junio C Hamano @ 2013-01-01 21:44 UTC (permalink / raw)
To: David A. Greene; +Cc: git, Jesper L. Nielsen
In-Reply-To: <1357012655-24974-7-git-send-email-greened@obbligato.org>
"David A. Greene" <greened@obbligato.org> writes:
> From: "Jesper L. Nielsen" <lyager@gmail.com>
>
> Before install git-subtree documentation, make sure the manpage
> directory exists.
>
> Author: Jesper L. Nielsen <lyager@gmail.com>
>
> Signed-off-by: Jesper L. Nielsen <lyager@gmail.com>
>
> Signed-off-by: David A. Greene <greened@obbligato.org>
> ---
> contrib/subtree/Makefile | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/contrib/subtree/Makefile b/contrib/subtree/Makefile
> index 36ae3e4..52d6fb9 100644
> --- a/contrib/subtree/Makefile
> +++ b/contrib/subtree/Makefile
> @@ -35,6 +35,7 @@ install: $(GIT_SUBTREE)
> install-doc: install-man
>
> install-man: $(GIT_SUBTREE_DOC)
> + mkdir -p $(man1dir)
We seem to use "$(INSTALL) -d -m 755" for this kind of thing (see
the Documentation/Makefile).
> $(INSTALL) -m 644 $^ $(DESTDIR)$(man1dir)
>
> $(GIT_SUBTREE_DOC): $(GIT_SUBTREE_XML)
^ permalink raw reply
* [PATCH 4/4] t9020: which is not portable
From: Torsten Bögershausen @ 2013-01-01 21:42 UTC (permalink / raw)
To: git; +Cc: tboegi
Use type instead
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
t/t9020-remote-svn.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t9020-remote-svn.sh b/t/t9020-remote-svn.sh
index 4f2dfe0..dbaecbc 100755
--- a/t/t9020-remote-svn.sh
+++ b/t/t9020-remote-svn.sh
@@ -32,8 +32,8 @@ fi
test_debug '
git --version
- which git
- which svnrdump
+ type git
+ type svnrdump
'
test_expect_success REMOTE_SVN 'simple fetch' '
--
1.8.0.197.g5a90748
^ permalink raw reply related
* [PATCH 4/4] t9020: which is not portable
From: Torsten Bögershausen @ 2013-01-01 21:42 UTC (permalink / raw)
To: git; +Cc: tboegi
Use type instead
Signed-off-by: Torsten Bögershausen <tboegi@web.de>
---
t/t9020-remote-svn.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t9020-remote-svn.sh b/t/t9020-remote-svn.sh
index 4f2dfe0..dbaecbc 100755
--- a/t/t9020-remote-svn.sh
+++ b/t/t9020-remote-svn.sh
@@ -32,8 +32,8 @@ fi
test_debug '
git --version
- which git
- which svnrdump
+ type git
+ type svnrdump
'
test_expect_success REMOTE_SVN 'simple fetch' '
--
1.8.0.197.g5a90748
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox