* 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: [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 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 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 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 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 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
* [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
* [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 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 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 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
* 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
* 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: [PATCH 1/8] Use %B for Split Subject/Body
From: Junio C Hamano @ 2013-01-02 0:30 UTC (permalink / raw)
To: greened; +Cc: git, Techlive Zheng
In-Reply-To: <87wqvwfsfm.fsf@waller.obbligato.org>
greened@obbligato.org writes:
> 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.
The question was about the lossage of the blank line, which does not
seem to be related to what this patch wants to do.
>>> -# 25
>>> +#25
>>
>> Why the lossage of a SP?
>
> I think this got fixed later in the series.
That is not a good excuse to introduce breakages in the first place, no?
>> 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.
That is what "tXXXX-*.sh -i" is for, isn't it?
^ permalink raw reply
* Re: [PATCH 2/8] Add --unannotate
From: Junio C Hamano @ 2013-01-02 0:32 UTC (permalink / raw)
To: greened; +Cc: git, James Nylen
In-Reply-To: <87sj6kfsbz.fsf@waller.obbligato.org>
greened@obbligato.org writes:
> In the meantime, will you apply the patch or do you prefer a new design?
The --unannotate option will become a baggage you will have to keep
working until the end of time, if we applied it. I think it is not
too uch a baggage, so it probably is OK.
^ permalink raw reply
* Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
From: Eric S. Raymond @ 2013-01-02 0:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vfw2k8t7k.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com>:
> 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?
Yes, they must install an updated cvsps. But this is hardly a loss, as
the old version was perilously broken.
There was an error or typo in the branch-analysis code, dating from
2006 and possibly earlier, that meant that branch root points would
almost always be attributed to parent patchsets one patchset earlier
than they should have been. Shocked me when I found it - how was this
missed for six years?
Because of the way the analysis is done, this fundamental bug would
also cause secondary damage like file changes near the root point
getting attributed to the wrong branch. In fact, this is how I
first spotted the problem; my test suite exhibited this symptom.
And mind you this is on top of ancestry-branch tracking not working -
two separate bugs that could interact in ways I'd really rather not
think about. The bottom line is that every import of a branchy CVS
repo with a pre-3.x version of cvsps is probably wrong.
The old git-cvsimport code was doing its part to screw things up, too.
At least three of the bugs on its manual page are problems I couldn't
reproduce using a bare cvsps instance, even the old broken version.
> 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?
Yes, but in that case I would strongly advise re-importing the entire
CVS history, as the portion analyzed with 2.2b1 and earlier versions
of cvsps will almost certainly have been somewhat garbled if it
contains any branches.
> 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?
I think you are being too conservative. This patch is *not* a mere
feature upgrade. The branch-analysis bug I found three days ago is not
a minor problem, it is a big ugly showstopper for any case beside the
simplest linear histories. Only linear histories will not break.
'People with existing set-ups' should absolutely *not* 'keep using the
old one'; we should yank that choice away from them and get the old
cvsimport/cvsps pair out of use *as fast as possible*, because it
silently mangles branchy imports.
Accordingly, giving people the idea that it's OK to use old and new
versions in parallel would be an extremely bad idea. I would go so
far as to call it irresponsible.
Here is what I have done to ease the transition:
If you try to use old git-cvsimport with new cvsps, new cvsps will detect
this and ship a message to stderr telling you to upgrade
If you try to use new git-cvsimport with old cvsps, old cvsps will complain
of an invalid argument and git-cvsimport will quit.
As for testing...cvsps now has several dozen self-tests on five
different CVS repositories, including improved versions of the
t960[123] tests. I will keep developing these as I work on bringing
parsecvs up to snuff.
I don't think there is a lot of point in git-cvsimport having its own
tests any more. If you read it I think you'll see why; it's a much
thinner wrapper around the conversion engine(s) than it used to be. In
particular, it no longer does its own protocol transactions to the
CVS server.
--
<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
^ permalink raw reply
* Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
From: Junio C Hamano @ 2013-01-02 1:06 UTC (permalink / raw)
To: esr; +Cc: git
In-Reply-To: <20130102003344.GA9651@thyrsus.com>
"Eric S. Raymond" <esr@thyrsus.com> writes:
> Junio C Hamano <gitster@pobox.com>:
>> 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?
>
> Yes, they must install an updated cvsps. But this is hardly a loss, as
> the old version was perilously broken.
>
> There was an error or typo in the branch-analysis code, dating from
> 2006 and possibly earlier, that meant that branch root points would
> almost always be attributed to parent patchsets one patchset earlier
> than they should have been. Shocked me when I found it - how was this
> missed for six years?
Would it be that not many people use branchy history in CVS, as
merging there was soooo painful?
>> Or am I being too conservative?
>
> I think you are being too conservative. This patch is *not* a mere
> feature upgrade. The branch-analysis bug I found three days ago is not
> a minor problem, it is a big ugly showstopper for any case beside the
> simplest linear histories. Only linear histories will not break.
That is exactly my point. It never worked in a branchy history, and
that is an indication that people who didn't complain and used the
old cvsimport with branch-incapable cvsps happily would have been
working with a linear history. Either nobody uses cvsimport in the
daily work, in which case a flag-day is perfectly fine, or we will
have many people who are forced to update to unproven version for no
immediate upside because the upstream repositories they work with, or
options they use cvsimport, do not trigger the multi-branch bug.
I however do understand that updating is the only sensible thing to
do for them *in the longer term*, as older cvsimport and cvsps are
no longer maintained, and sooner they update the better the chance
the new cvsimport becomes perfect earlier.
> 'People with existing set-ups' should absolutely *not* 'keep using the
> old one'; we should yank that choice away from them and get the old
> cvsimport/cvsps pair out of use *as fast as possible*, because it
> silently mangles branchy imports.
I still am not convinced, especially without a "we make sure we do
not regress in linear histories" side-by-side test in place. That
sounds irresponsible.
But others may disagree, and I'd have to sleep on it.
I'd prefer to hear from somebody who is *not* defending on his newer
implementation, but from somebody who is actively using cvsimport as
an end user. On the end-users' side, there always is this anxiety
that a radical rewrite will always introduce new bugs, even when
they know the rewrite is done very competently.
> Here is what I have done to ease the transition:
>
> If you try to use old git-cvsimport with new cvsps, new cvsps will detect
> this and ship a message to stderr telling you to upgrade
Sounds sensible.
> If you try to use new git-cvsimport with old cvsps, old cvsps will complain
> of an invalid argument and git-cvsimport will quit.
With an error message that tells the user to update cvsps, this also
sounds sensible.
^ permalink raw reply
* Makefile dependency from 'configure' to 'GIT-VERSION-FILE'
From: Martin von Zweigbergk @ 2013-01-02 1:11 UTC (permalink / raw)
To: Stefano Lattarini, Jeff King, Jonathan Nieder; +Cc: git
Hi,
I use autoconf with git.git. I have noticed lately, especially when
doing things like "git rebase -i --exec make", that ./configure is run
every time. If I understand correctly, this is because of 8242ff4
(build: reconfigure automatically if configure.ac changes,
2012-07-19). Just a few days before that commit, on 2012-07-15, the
branch jn/makefile-cleanup including 520a6cd (Makefile: move
GIT-VERSION-FILE dependencies closer to use, 2012-06-20) was merged
(to next?). I wonder if these two subjects were aware of each other.
The reason 'configure' depends on GIT-VERSION-FILE is because it
inserts the version into the call to AC_INIT. I have close to no
experience with autoconf or even make and it's not at all clear to me
why we need to pass the verison to AC_INIT. It seems like it's just
for messages printed by ./configure. If that's the case, we shouldn't
need to generate a new 'configure' file ever time. At the very least,
we shouldn't need to run it.
Do you think we should simply remove the dependency from 'configure'
to 'GIT-VERSION-FILE' and leave a comment there instead? Or should we
instead somehow make 'reconfigure' depend only on 'configure.ac'? Both
of these feel a little wrong to me, because they would remove real
dependencies. Maybe the (probably mangled) patch at the end of this
message is better?
Martin
diff --git a/Makefile b/Makefile
index 736ecd4..ec5d7ca 100644
--- a/Makefile
+++ b/Makefile
@@ -2267,12 +2267,9 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : unimplemented.sh
mv $@+ $@
endif # NO_PYTHON
-configure: configure.ac GIT-VERSION-FILE
- $(QUIET_GEN)$(RM) $@ $<+ && \
- sed -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
- $< > $<+ && \
- autoconf -o $@ $<+ && \
- $(RM) $<+
+configure: configure.ac
+ $(QUIET_GEN)$(RM) $@ && \
+ autoconf -o $@ $<
ifdef AUTOCONFIGURED
config.status: configure
diff --git a/configure.ac b/configure.ac
index ad215cc..00c3e38 100644
--- a/configure.ac
+++ b/configure.ac
@@ -142,7 +142,10 @@ fi
## Configure body starts here.
AC_PREREQ(2.59)
-AC_INIT([git], [@@GIT_VERSION@@], [git@vger.kernel.org])
+AC_INIT([git],
+ m4_esyscmd([ ./GIT-VERSION-GEN &&
+ { sed -ne 's/GIT_VERSION = //p' GIT-VERSION-FILE
| xargs echo -n; } ]),
+ [git@vger.kernel.org])
AC_CONFIG_SRCDIR([git.c])
^ permalink raw reply related
* Re: [PATCH 3/4] t4014: do not uese echo -n
From: Junio C Hamano @ 2013-01-02 1:16 UTC (permalink / raw)
To: Brandon Casey; +Cc: git, Torsten Bögershausen
In-Reply-To: <201301012241.17050.tboegi@web.de>
Torsten Bögershausen <tboegi@web.de> writes:
> echo -n is not portable on all systems.
> Use printf instead
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
> ---
Brandon, this comes from 932581b (Unify appending signoff in
format-patch, commit and sequencer, 2012-11-25). Please make sure
to squash it in when you reroll the series.
Thanks (and a happy new year ;-).
> t/t4014-format-patch.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index 6cfad13..f460930 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -993,7 +993,7 @@ EOF
> '
>
> test_expect_success 'signoff: commit with only subject that does not end with NL' '
> - echo -n subject | append_signoff >actual &&
> + printf subject | append_signoff >actual &&
> cat >expected <<\EOF &&
> 4:Subject: [PATCH] subject
> 8:
^ permalink raw reply
* Re: [PATCH 2/3] format-patch: pick up branch description when no ref is specified
From: Duy Nguyen @ 2013-01-02 1:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7v38ykbpv3.fsf@alter.siamese.dyndns.org>
On Wed, Jan 2, 2013 at 3:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> find_branch_name() fails to detect "format-patch --cover-letter -3"
>> where no command line arguments are given and HEAD is automatically
>> added.
>
> Nicely spotted.
>
> That is not the only case that takes this codepath, though.
>
> $ git format-patch --cover-letter master..
>
> will also give you the same (if you say it without "..", which is
> the more normal invocation of the command, then the caller already
> know you meant the current branch and this function is not called).
>
> And in that case you will have two tokens on cmdline.nr, one for
> "master.." to show where he bottom is, and the other for the
> implied "HEAD";
Interesting. find_brach_name() handles this case wrong because
rev->cmdline[positive].name is "HEAD" and it goes ahead prepending
"refs/heads/" anyway. I'll fix it in the reroll. I was avoiding tests
with an excuse that you did not write tests when you added this
function either. But with this change, I think tests are required.
> I do not think this patch is a sufficient solution
> for the more general cases, but on the other hand I do not know how
> much it matters.
I think the best place to handle this is setup_revisions() for general
cases. But we do not need branch detection anywhere else yet, I guess
it's ok to fix it case by case here. We can move the code back to
revisions.c when there are more use cases for it.
>> - if (positive < 0)
>> + if (positive < 0) {
>> + /*
>> + * No actual ref from command line, but "HEAD" from
>> + * rev->def was added in setup_revisions()
>> + * e.g. format-patch --cover-letter -12
>> + */
>
> That comment does not describe "positive < 0" case, but belongs to
> the conditional added in this patch, no?
It's meant as the comment for the following block, yes. I'll move it
into the block for clarity.
>> + if (!rev->cmdline.nr &&
>> + rev->pending.nr == 1 &&
>> + !strcmp(rev->pending.objects[0].name, "HEAD")) {
>> + const char *ref;
>> + ref = resolve_ref_unsafe("HEAD", branch_sha1, 1, NULL);
>> + if (ref && !prefixcmp(ref, "refs/heads/"))
>> + return xstrdup(ref + strlen("refs/heads/"));
>> + }
>> return NULL;
>> + }
>> strbuf_addf(&buf, "refs/heads/%s", rev->cmdline.rev[positive].name);
>> branch = resolve_ref_unsafe(buf.buf, branch_sha1, 1, NULL);
>> if (!branch ||
--
Duy
^ permalink raw reply
* Re: Bug in latest gitk - can't click lines connecting commits
From: Paul Mackerras @ 2013-01-01 23:22 UTC (permalink / raw)
To: Stefan Haller; +Cc: Jason Holden, git
In-Reply-To: <1kw18d3.5sftkl125qdz4M%stefan@haller-berlin.de>
On Tue, Jan 01, 2013 at 06:54:23PM +0100, Stefan Haller wrote:
> Jason Holden <jason.k.holden.swdev@gmail.com> wrote:
>
> > I was testing some patches against the latest gitk, and noticed that when I
> > click the mouse on the lines that connect the commits in the history graph,
> > I get an error popup with:
> > Error: can't read "cflist_top": no such variable
> >
> > Looks like this was introduced in gitk commit b967135d89e8d8461d059
> > gitk: Synchronize highlighting in file view when scrolling diff
>
> A patch that fixes this was proposed over two months ago, and Paul said
> he had applied it:
>
> <http://permalink.gmane.org/gmane.comp.version-control.git/208162>
>
> However, looking at git://ozlabs.org/~paulus/gitk.git it's not there.
> Paul?
I just forgot to push it out. It's there now.
Paul.
^ permalink raw reply
* [PATCH v3] git-clean: Display more accurate delete messages
From: Zoltan Klinger @ 2013-01-02 1:45 UTC (permalink / raw)
To: git; +Cc: Zoltan Klinger
(1) Only print out the names of the files and directories that got
actually deleted.
(2) Show warning message for ignored untracked git repositories
Consider the following repo layout:
test.git/
|-- tracked_dir/
| |-- some_tracked_file
| |-- some_untracked_file
|-- tracked_file
|-- untracked_file
|-- untracked_foo/
| |-- bar/
| | |-- bar.txt
| |-- emptydir/
| |-- frotz.git/
| |-- frotz.tx
|-- untracked_some.git/
|-- some.txt
Suppose the user issues 'git clean -fd' from the test.git directory.
When -d option is used and untracked directory 'foo' contains a
subdirectory 'frotz.git' that is managed by a different git repository
therefore it will not be removed.
$ git clean -fd
Removing tracked_dir/some_untracked_file
Removing untracked_file
Removing untracked_foo/
Removing untracked_some.git/
The message displayed to the user is slightly misleading. The foo/
directory has not been removed because of foo/frotz.git still exists.
On the other hand the subdirectories 'bar' and 'emptydir' have been
deleted but they're not mentioned anywhere. Also, untracked_some.git
has not been removed either.
This behaviour is the result of the way the deletion of untracked
directories are reported. In the current implementation they are
deleted recursively but only the name of the top most directory is
printed out. The calling function does not know about any
subdirectories that could not be removed during the recursion.
Improve the way the deleted directories are reported back to
the user:
(1) Create a recursive delete function 'remove_dirs' in builtin/clean.c
to run in both dry_run and delete modes with the delete logic as
follows:
(a) Check if the current directory to be deleted is an untracked
git repository. If it is and --force --force option is not set
do not touch this directory, print ignore message, set dir_gone
flag to false for the caller and return.
(b) Otherwise for each item in current directory:
(i) If current directory cannot be accessed, print warning,
set dir_gone flag to false and return.
(ii) If the item is a subdirectory recurse into it,
check for the returned value of the dir_gone flag.
If the subdirectory is gone, add the name of the deleted
directory to a list of successfully removed items 'dels'.
Else set the dir_gone flag as the current directory
cannot be removed because we have at least one subdirectory
hanging around.
(iii) If it is a file try to remove it. If success add the
file name to the 'dels' list, else print error and set
dir_gone flag to false.
(c) After we finished deleting all items in the current directory and
the dir_gone flag is still true, remove the directory itself.
If failed set the dir_gone flag to false.
(d) If the current directory cannot be deleted because the dir_gone flag
has been set to false, print out all the successfully deleted items
for this directory from the 'dels' list.
(e) We're done with the current directory, return.
(2) Modify the cmd_clean() function to:
(a) call the recursive delete function 'remove_dirs()' for each
topmost directory it wants to remove
(b) check for the returned value of dir_gone flag. If it's true
print the name of the directory as being removed.
Consider the output of the improved version:
$ git clean -fd
Removing tracked_dir/some_untracked_file
Removing untracked_file
warning: ignoring untracked git repository untracked_foo/frotz.git
Removing untracked_foo/bar
Removing untracked_foo/emptydir
warning: ignoring untracked git repository untracked_some.git/
Now it displays only the file and directory names that got actually
deleted and shows warnings about ignored untracked git repositories.
Reported-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
Signed-off-by: Zoltan Klinger <zoltan.klinger@gmail.com>
---
builtin/clean.c | 149 ++++++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 120 insertions(+), 29 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 69c1cda..37e403a 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -10,6 +10,7 @@
#include "cache.h"
#include "dir.h"
#include "parse-options.h"
+#include "refs.h"
#include "string-list.h"
#include "quote.h"
@@ -20,6 +21,13 @@ static const char *const builtin_clean_usage[] = {
NULL
};
+static const char* MSG_REMOVE = "Removing %s\n";
+static const char* MSG_WOULD_REMOVE = "Would remove %s\n";
+static const char* MSG_WOULD_NOT_REMOVE = "Would not remove %s\n";
+static const char* MSG_WOULD_IGNORE_GIT_DIR = "Would ignore untracked git repository %s\n";
+static const char* MSG_WARN_GIT_DIR_IGNORE = "ignoring untracked git repository %s";
+static const char* MSG_WARN_REMOVE_FAILED = "failed to remove %s";
+
static int git_clean_config(const char *var, const char *value, void *cb)
{
if (!strcmp(var, "clean.requireforce"))
@@ -34,11 +42,109 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
return 0;
}
+static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
+ int dry_run, int quiet, int *dir_gone)
+{
+ DIR *dir;
+ struct strbuf quoted = STRBUF_INIT;
+ struct dirent *e;
+ int res = 0, ret = 0, gone = 1, original_len = path->len, len, i;
+ unsigned char submodule_head[20];
+ struct string_list dels = STRING_LIST_INIT_DUP;
+
+ *dir_gone = 1;
+
+ quote_path_relative(path->buf, strlen(path->buf), "ed, prefix);
+ if ((force_flag & REMOVE_DIR_KEEP_NESTED_GIT) &&
+ !resolve_gitlink_ref(path->buf, "HEAD", submodule_head)) {
+ if (dry_run && !quiet)
+ printf(_(MSG_WOULD_IGNORE_GIT_DIR), quoted.buf);
+ else if (!dry_run)
+ warning(_(MSG_WARN_GIT_DIR_IGNORE), quoted.buf);
+
+ *dir_gone = 0;
+ return 0;
+ }
+
+ dir = opendir(path->buf);
+ if (!dir) {
+ /* an empty dir could be removed even if it is unreadble */
+ res = dry_run ? 0 : rmdir(path->buf);
+ if (res) {
+ warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
+ *dir_gone = 0;
+ }
+ return res;
+ }
+
+ if (path->buf[original_len - 1] != '/')
+ strbuf_addch(path, '/');
+
+ len = path->len;
+ while ((e = readdir(dir)) != NULL) {
+ struct stat st;
+ if (is_dot_or_dotdot(e->d_name))
+ continue;
+
+ strbuf_setlen(path, len);
+ strbuf_addstr(path, e->d_name);
+ quote_path_relative(path->buf, strlen(path->buf), "ed, prefix);
+ if (lstat(path->buf, &st))
+ ; /* fall thru */
+ else if (S_ISDIR(st.st_mode)) {
+ if (remove_dirs(path, prefix, force_flag, dry_run, quiet, &gone))
+ ret = 1;
+ if (gone)
+ string_list_append(&dels, quoted.buf);
+ else
+ *dir_gone = 0;
+ continue;
+ } else {
+ res = dry_run ? 0 : unlink(path->buf);
+ if (!res)
+ string_list_append(&dels, quoted.buf);
+ else {
+ warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
+ *dir_gone = 0;
+ ret = 1;
+ }
+ continue;
+ }
+
+ /* path too long, stat fails, or non-directory still exists */
+ *dir_gone = 0;
+ ret = 1;
+ break;
+ }
+ closedir(dir);
+
+ strbuf_setlen(path, original_len);
+ quote_path_relative(path->buf, strlen(path->buf), "ed, prefix);
+
+ if (*dir_gone) {
+ res = dry_run ? 0 : rmdir(path->buf);
+ if (!res)
+ *dir_gone = 1;
+ else {
+ warning(_(MSG_WARN_REMOVE_FAILED), quoted.buf);
+ *dir_gone = 0;
+ ret = 1;
+ }
+ }
+
+ if (!*dir_gone && !quiet) {
+ for (i = 0; i < dels.nr; i++)
+ printf(dry_run ? _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), dels.items[i].string);
+ }
+ string_list_clear(&dels, 0);
+ return ret;
+}
+
int cmd_clean(int argc, const char **argv, const char *prefix)
{
- int i;
- int show_only = 0, remove_directories = 0, quiet = 0, ignored = 0;
- int ignored_only = 0, config_set = 0, errors = 0;
+ int i, res;
+ int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
+ int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
struct strbuf directory = STRBUF_INIT;
struct dir_struct dir;
@@ -49,7 +155,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
char *seen = NULL;
struct option options[] = {
OPT__QUIET(&quiet, N_("do not print names of files removed")),
- OPT__DRY_RUN(&show_only, N_("dry run")),
+ OPT__DRY_RUN(&dry_run, N_("dry run")),
OPT__FORCE(&force, N_("force")),
OPT_BOOLEAN('d', NULL, &remove_directories,
N_("remove whole directories")),
@@ -77,7 +183,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
if (ignored && ignored_only)
die(_("-x and -X cannot be used together"));
- if (!show_only && !force) {
+ if (!dry_run && !force) {
if (config_set)
die(_("clean.requireForce set to true and neither -n nor -f given; "
"refusing to clean"));
@@ -150,38 +256,23 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
if (S_ISDIR(st.st_mode)) {
strbuf_addstr(&directory, ent->name);
qname = quote_path_relative(directory.buf, directory.len, &buf, prefix);
- if (show_only && (remove_directories ||
- (matches == MATCHED_EXACTLY))) {
- printf(_("Would remove %s\n"), qname);
- } else if (remove_directories ||
- (matches == MATCHED_EXACTLY)) {
- if (!quiet)
- printf(_("Removing %s\n"), qname);
- if (remove_dir_recursively(&directory,
- rm_flags) != 0) {
- warning(_("failed to remove %s"), qname);
+ if (remove_directories || (matches == MATCHED_EXACTLY)) {
+ if (remove_dirs(&directory, prefix, rm_flags, dry_run, quiet, &gone))
errors++;
- }
- } else if (show_only) {
- printf(_("Would not remove %s\n"), qname);
- } else {
- printf(_("Not removing %s\n"), qname);
+ if (gone && !quiet)
+ printf(dry_run ? _(MSG_WOULD_REMOVE) : _(MSG_REMOVE), qname);
}
strbuf_reset(&directory);
} else {
if (pathspec && !matches)
continue;
qname = quote_path_relative(ent->name, -1, &buf, prefix);
- if (show_only) {
- printf(_("Would remove %s\n"), qname);
- continue;
- } else if (!quiet) {
- printf(_("Removing %s\n"), qname);
- }
- if (unlink(ent->name) != 0) {
- warning(_("failed to remove %s"), qname);
+ res = dry_run ? 0 : unlink(ent->name);
+ if (res) {
+ warning(_(MSG_WARN_REMOVE_FAILED), qname);
errors++;
- }
+ } else if (!quiet)
+ printf(dry_run ? _(MSG_WOULD_REMOVE) :_(MSG_REMOVE), qname);
}
}
free(seen);
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories
From: Jason Holden @ 2013-01-02 1:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <1357082695-29713-3-git-send-email-gitster@pobox.com>
On Tue, Jan 01, 2013 at 03:24:54PM -0800, Junio C Hamano wrote:
>
> ------------------------------------------------
> +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
>
Any reason to leave out the maintainers email addresses? If we add that, all
the content of the MAINTAINERS file we had been discussing would then be in
this file.
My only other suggestion for this series might be to augment the file with
a patch submittal example(s). I found the best example to be in
git-send-email's man page, but maybe enumerate the example out for the
most common workflows. Maybe something like:
-----------------------------------------------------------
Example of sending patches for a new feature:
Create the patches:
$ git format-patch --cover-letter -M origin/master -o outgoing/
$ edit outgoing/0000-*
To send your completed patches for initial consideration:
$ git send-email outgoing/* -to git@vger.kernel.org -cc gitster@pobox.com
To send your reviewed patches for inclusion:
$ git send-email outgoing/* -to gitster@pobox.com -cc git@vger.kernel.org
-Jason
^ permalink raw reply
* Re: [PATCH 2/3] SubmittingPatches: mention subsystems with dedicated repositories
From: Junio C Hamano @ 2013-01-02 2:14 UTC (permalink / raw)
To: Jason Holden; +Cc: git
In-Reply-To: <20130102015233.GA25288@gmail.com>
Jason Holden <jason.k.holden.swdev@gmail.com> writes:
> Any reason to leave out the maintainers email addresses?
Nothing particular, other than that I did not find anywhere in the
file that does not break the flow.
> My only other suggestion for this series might be to augment the file with
> a patch submittal example(s). I found the best example to be in
> git-send-email's man page,...
I'd feel better to avoid copying and pasting. If send-email has
good examples, shouldn't it be sufficient to refer to them?
> -----------------------------------------------------------
> Example of sending patches for a new feature:
>
> Create the patches:
> $ git format-patch --cover-letter -M origin/master -o outgoing/
> $ edit outgoing/0000-*
>
> To send your completed patches for initial consideration:
> $ git send-email outgoing/* -to git@vger.kernel.org -cc gitster@pobox.com
This is ambiguous; it makes it look as if you are CC'ing the
maintainer, but for the initial round, it is likely that you are
sending it to me only because I have been involved in the area the
patch touches.
> To send your reviewed patches for inclusion:
> $ git send-email outgoing/* -to gitster@pobox.com -cc git@vger.kernel.org
This is fine, but we would probably want to see it CC'ed to people
who reviewed the initial submission, too.
^ 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