* Re: git.c option parsing
From: Junio C Hamano @ 2007-12-17 8:48 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: git
In-Reply-To: <F6F3247E-4E71-4977-9626-F0571278E1E6@wincent.com>
Wincent Colaiuta <win@wincent.com> writes:
> Of course, the above plan will only work for builtins, not for
> scripts. An additional step would be needed to enable scripts to
> handle these options; perhaps teaching "git rev-parse" something...
As long as special options stay special and we make a rule not to allow
any subcommand to assign its own meaning to them, the git wrapper can
lookahead and reorder, when seeing a command line:
git scripted-command --special
into
git --special scripted-command
And that approach would work well for built-ins as well, I would
imagine.
There is one minor detail, though. There could be an option-parameter
that is literally --special. E.g.
git grep -e --no-paginate
should not be reordered to
git --no-paginate grep -e
^ permalink raw reply
* Re: [PATCH] Re-re-re-fix common tail optimization
From: Wincent Colaiuta @ 2007-12-17 8:42 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Linus Torvalds, git
In-Reply-To: <20071216222919.GA2260@coredump.intra.peff.net>
El 16/12/2007, a las 23:29, Jeff King escribió:
> On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:
>
>> Yeah, I amended it without adding another "re-" to the title ;-) The
>> result has been already pushed out.
>
> OK. Too late, but it has my ack. ;)
>
>>> Aren't we using "git diff" for the second diff there nowadays?
>>
>> Some people seem to think that is a good idea, but I generally do not
>> like using "git diff" between expect and actual (both untracked)
>> inside
>> tests. The last "diff" is about validating what git does and using
>> "git
>> diff" there would make the test meaningless when "git diff" itself is
>> broken.
>
> I think that is a valid concern. But ISTR that were some issues with
> using GNU diff. Commit 5bd74506 mentions getting rid of the dependency
> in all existing tests, but gives no reason.
I'd say it's safe and sensible to use "git diff" in all tests *except*
for tests of "git diff" itself.
Wincent
^ permalink raw reply
* What's cooking in git.git (topics)
From: Junio C Hamano @ 2007-12-17 8:40 UTC (permalink / raw)
To: git
In-Reply-To: <7vabof5mze.fsf@gitster.siamese.dyndns.org>
Here are the topics that have been cooking. Commits prefixed
with '-' are only in 'pu' while commits prefixed with '+' are
in 'next'. Others commits may be stashed in 'offcuts'.
The topics list the commits in reverse chronological order.
----------------------------------------------------------------
[New Topics]
* ph/parseopt (Sun Dec 16 22:45:00 2007 -0800) 4 commits
- builtin-tag: fix fallouts from recent parsopt restriction.
- (squashme) gitcli documentation fixups
- parseopt: Add a gitcli(5) man page.
- parseopt: Enforce the use of the sticked form for optional
arguments.
This series is needed by 1.5.4-rc1 for fixing regression relative to
1.5.3 series in the option parser: "git describe --abbrev HEAD" does not
work. The current approach is taken by this series (discussed on the
list) to work it around by forbidding "git describe --abbrev 10 HEAD"
and requiring it be written as "git describe --abbrev=10 HEAD", but
taking that limitation literally will introduce serious usability
regressions. We need careful auditing of all the commands that adopted
parse_options() API.
* ab/pserver (Fri Dec 14 04:08:51 2007 +0000) 1 commit
- Authentication support for pserver
This came somewhat late; I think cvsserver is in its own little corner
and the change seems to be quite isolated, so with enough user requests
and Ack from primary people who have been involved with cvsserver I do
not think we mind to make an exception and make it a part of 1.5.4-rc1.
----------------------------------------------------------------
[Graduated to 'master']
* mh/http (Tue Dec 11 00:08:25 2007 +0100)
Big thanks to Daniel who helped reviewing this series which is about
clean-ups and fixes in http transport.
----------------------------------------------------------------
[Will cook further in 'next' and then merge to 'master' soon]
Again, we are post -rc0 and there is nothing interesting to see here.
----------------------------------------------------------------
[Actively cooking]
Again, we are post -rc0 and there is nothing interesting to see here.
----------------------------------------------------------------
[On hold]
* jc/api-doc (Sat Nov 24 23:48:04 2007 -0800) 1 commit
The primary reason of this series is because I think we made the system
a lot less approachable by losing hackability. Although we still have
sample scripts in contrib/example for use of plumbing in scripts, they
will not help aspiring git-hacker-wannabees when our primary attention
has already shifted to moving things to C.
This currently consists of mostly stubs, although I wrote about a few
topics as examples. Nice to have in v1.5.4, but we need more writers.
* nd/dashless (Wed Nov 28 23:21:57 2007 +0700) 1 commit
- Move all dashed-form commands to libexecdir
I think this is a sane thing to do in the longer term. Will be in
'next' after v1.5.4. I think "leave porcelain on PATH" might be also a
good thing as a transition measure.
Incidentally, if we do not install dashed form of built-ins anywhere
(which is not this series is about --- this is just moving them out of
user's PATH), "git help -a" will stop showing them. I am not enthused
about removing the hardlinks to built-ins to begin with, but people who
want such a change need to first modify help.c:list_commands() to pick
up builtins without having git-foo hardlinks in gitexecdir. This may
need to happen anyway as mingw fallouts.
* js/remote (Wed Dec 5 19:02:15 2007 +0000) 4 commits
- Make git-remote a builtin
- Test "git remote show" and "git remote prune"
- parseopt: add flag to stop on first non option
- path-list: add functions to work with unsorted lists
This and Kristian's "git-clone in C" are on hold and post 1.5.4.
----------------------------------------------------------------
[Stalled]
* ns/checkout-push-pop (Wed Dec 5 07:04:06 2007 +0900) 1 commit
- git-checkout --push/--pop
A reasonably cleanly written cute hack, and I do not see this breaking
the normal codepath. But I tend to agree with people that 'push' is
too late for forgetful mortals, and just a single "previous" would be
easier to use.
* jc/git-symref (Tue Dec 11 16:42:46 2007 -0800) 1 commit
- PARK: show-symref protocol extension.
This is a demonstration of a possible component in the future direction
for HEAD discovery done by git-clone.
* js/reflog-delete (Wed Oct 17 02:50:45 2007 +0100) 1 commit
+ Teach "git reflog" a subcommand to delete single entries
* jk/builtin-alias (Fri Nov 30 11:22:58 2007 -0500) 1 commit
+ Support builtin aliases
Even the original author has slight NAK on this and I tend to agree.
May want to eventurally revert from 'next' but we are not in a hurry
even to do that.
* jc/diff-pathspec (Sun Nov 25 10:03:48 2007 -0800) 1 commit
- Making ce_path_match() more useful by accepting globs
This was to allow "git diff-files -- '*.h'" (currently diff family
knows only the leading directory match and not fileglobs), but was shot
down by Alex. I tend to agree with him.
* jc/dashless (Sat Dec 1 22:09:22 2007 -0800) 2 commits
- Prepare execv_git_cmd() for removal of builtins from the
filesystem
- git-shell: accept "git foo" form
We do not plan to remove git-foo form completely from the filesystem at
this point, so these are not strictly necessary.
* jc/diff-relative (Thu Dec 6 09:48:32 2007 -0800) 1 commit
. Make "diff" Porcelain output paths as relative to subdirectory.
* jc/pathspec (Thu Sep 13 13:38:19 2007 -0700) 3 commits
. pathspec_can_match(): move it from builtin-ls-tree.c to tree.c
. ls-tree.c: refactor show_recursive() and rename it.
. tree-diff.c: split out a function to match a single pattern.
* jc/cherry-pick (Sun Dec 16 21:00:03 2007 -0800) 2 commits
. beginning of use of replay merge in revert
. revert/cherry-pick: start refactoring call to merge_recursive
* jc/nu (Sun Oct 14 22:07:34 2007 -0700) 3 commits
. merge-nu: a new merge backend without using unpack_trees()
. read_tree: take an explicit index structure
. gcc 4.2.1 -Werror -Wall -ansi -pedantic -std=c99: minimum fix
^ permalink raw reply
* What's in git.git (stable frozen)
From: Junio C Hamano @ 2007-12-17 8:40 UTC (permalink / raw)
To: git
In-Reply-To: <7vbq8v5n0u.fsf_-_@gitster.siamese.dyndns.org>
Sorry for the "\No newline at the end" brown paper bag bug, which
happened soon after -rc0 but now fixed. Thanks Jeff.
"git diff --check" was inconsistent with the highlighting of whitespace
problems and was harder to use from scripts (which makes it a usability
bug). Both have been fixed. Thanks Wincent.
A handful "gitweb" fixes I forgot to apply earlier are now in. Thanks
Jakub.
A "git commit" regression has been noticed --- the post-commit summary
was inconsistent with what is shown by git-status before making the
commit. Thanks Daniel.
A workaround for massive commit that exceeds command line limit when
issuing "cvs status list-of-files" has been added. Thanks Jeff.
Hopefully the final round of clean-up until 1.5.4 in whitespace area has
been applied. Thanks Wincent and Bruce.
I am reasonably happy that we are finding and fixing bugs. Please keep
the fixes, including documentation updates, flowing in.
* "git -p ls-tree HEAD" in a bare repository is still not fixed.
http://thread.gmane.org/gmane.comp.version-control.git/68209?focus=68221
http://thread.gmane.org/gmane.comp.version-control.git/68209?focus=68338
* Hannes says "thread pack-objects" improvements needs a follow-up
patch; it hasn't been applied here yet.
* option parser regression fix really needs to be in by -rc1, but I
just applied the initial round of Pierre's patches today. See
"What's cooking" for the list.
I need to coordinate with subsystem people and pull in changes for
gitk, git-gui, user manual and git-svn.
----------------------------------------------------------------
* The 'master' branch has these since the last announcement
in addition to the above.
Andy Whitcroft (1):
git-svn: expand handling of From: and Signed-off-by:
Christian Couder (1):
git-help: add "help.format" config variable.
David S. Miller (1):
fast-import: fix unalinged allocation and access
Eric Wong (2):
git-svn: unlink internal index files after operations
git-svn: handle our top-level path being deleted and later re-added
J. Bruce Fields (8):
whitespace: fix off-by-one error in non-space-in-indent checking
whitespace: reorganize initial-indent check
whitespace: minor cleanup
whitespace: fix initial-indent checking
whitespace: more accurate initial-indent highlighting
whitespace: fix config.txt description of indent-with-non-tab
builtin-apply: minor cleanup of whitespace detection
builtin-apply: stronger indent-with-on-tab fixing
Jakub Narebski (2):
gitweb: disambiguate heads and tags withs the same name
gitweb: Teach "a=blob" action to be more lenient about blob/file mime
type
Jeff King (4):
cvsexportcommit: fix massive commits
trim_common_tail: brown paper bag fix.
teach bash completion to treat commands with "--" as a helper
rename git-browse--help to git-help--browse
Johannes Sixt (1):
threaded pack-objects: Use condition variables for thread communication.
Junio C Hamano (15):
git-commit: squelch needless message during an empty merge
xdl_diff: identify call sites.
xdi_diff: trim common trailing lines
diff --check: minor fixups
xdiff tail trimming: use correct type.
commit: allow --amend to reuse message from another commit
remote: Fix bogus make_branch() call in configuration reader.
Rename git-browse-help helper to git-browse--help
Retire git-runstatus for real.
Start preparing the API documents.
Update draft release notes for 1.5.4
Re-re-re-fix common tail optimization
builtin-commit: fix summary output.
builtin-commit: make summary output consistent with status
Documentation/git-submodule: refer to gitmodules(5)
Kristian Høgsberg (1):
Use a strbuf for building up section header and key/value pair strings.
Li Hong (1):
Fix a memory leak
Mike Hommey (8):
Fix some more memory leaks in http-push.c
Fix random sha1 in error message in http-fetch and http-push
Remove the default_headers variable from http-push.c
Remove a CURLOPT_HTTPHEADER (un)setting
Avoid redundant declaration of missing_target()
Use strbuf in http code
Fix various memory leaks in http-push.c and http-walker.c
Move fetch_ref from http-push.c and http-walker.c to http.c
Nicolas Pitre (1):
provide advance warning of some future pack default changes
Petr Baudis (1):
gitweb: Make config_to_multi return [] instead of [undef]
Shawn Bohrer (1):
Fix spelling mistakes in user manual
Wincent Colaiuta (6):
"diff --check" should affect exit status
Unify whitespace checking
Make "diff --check" output match "git apply"
Add tests for "git diff --check" with core.whitespace options
Use shorter error messages for whitespace problems
Test interaction between diff --check and --exit-code
anonymous (1):
Documentation: describe pack idx v2
^ permalink raw reply
* git.c option parsing (was: [PATCH 1/3] Revert changes and extend diff option documentation)
From: Wincent Colaiuta @ 2007-12-17 8:27 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vejdml92c.fsf@gitster.siamese.dyndns.org>
El 16/12/2007, a las 20:43, Junio C Hamano escribió:
> Wincent Colaiuta <win@wincent.com> writes:
>
>> diff --git a/Documentation/diff-options.txt b/Documentation/diff-
>> options.txt
>> index 9ecc1d7..54207f0 100644
>> --- a/Documentation/diff-options.txt
>> +++ b/Documentation/diff-options.txt
>> @@ -92,10 +92,10 @@ endif::git-format-patch[]
>> file gives the default to do so.
>>
>> --check::
>> - Warn if changes introduce trailing whitespace
>> - or an indent that uses a space before a tab. Exits with
>> - non-zero status if problems are found. Not compatible with
>> - --exit-code.
>> + Warn if changes introduce whitespace problems (such as
>> + trailing whitespace). Configuration and per-path attributes
>> + control what git classifies as a whitespace problem (see
>> + gitlink:git-config[1] and gitlink:gitattributes[5]).
>
> This is not quite right, is it? The command still exits with exit
> code.
> It is just that the calling process does not see it if you let it
> spawn
> the pager.
Yes, I know, but I thought that in the interests of brevity it would
be best not to clutter up the man page with that kind of detail. If a
user is interested in the exit code they will probably do a search of
the man page for "exit" and find the --exit-code section, which gives
them the answer they want. Adding a line like "Also, if you pass --no-
pager will exit with a non-zero exit code" doesn't really add much
other than clutter IMO.
Incidentally, and this is really tangential, I find the whole "--no-
pager" UI pretty horrendous, along with all the other switches parsed
by git.c. Basically, understanding these switches requires the user to
know internal implementation details that he/she should most
definitely not have to know.
For example, "git --no-pager diff --check" works as expected but "git
diff --no-pager --check" carps with "error: invalid option: --no-
pager". To understand why this is so, the user needs to know too many
things which should be internal implementation details:
- that "git" is just a wrapper tool that chooses a low level command
to run and runs it
- that there are some parameters that affect the way all the other
commands run and should be passed to "git" only, prior to the
"subcommand" part
- that those parameters can't be passed after the "subcommand"
specification
And seeing as we are actively encouraging users to always use the "git
diff" form rather than "git-diff", in a sense we are actively
encourage them to think of Git as a single tool rather than a
collection of commands, it seems like an ugly wart that we then have
to teach them that some parameters must go *between* the "git" and the
"diff" but not after. And of course, if you always use the "git-diff"
then there's no way to use the --no-pager switch at all; you instead
have to use an environment variable.
I've been thinking about ways to iron out these wrinkles and unify
things. Here's my thinking about a possible cause of action:
1. Factor out the code in git.c which handles the common arguments
into separate functions; at this stage it's just a refactoring and the
behaviour is unchanged, but those separate functions will later be
used from the builtins.
2. Teach the builtin commands to handle those common arguments by
using the common functions. For example, we teach "git diff" to
understand the --no-pager option by leveraging the functions we just
factored out in the previous step.
3. Now, when git.c sees an argument that would normally go before a
"subcommand" it no longer needs to handle it directly itself but can
instead pass it along to the subcommand. In other words, where it
formerly saw "git --no-pager diff args..." and handled the --no-pager
part itself, it can instead just pass "--no-pager args..." to the
builtin diff. This gives us backward compatibility with the old
format, but the new format (user types "git diff --no-pager args...")
will work also.
4. Update the docs: make a common-options.txt page which is included
in all other manual pages either listing the options explicitly or
directing users to the git(7) manpage (which should probably become
git(1) if we are talking about Git as a single tool) to learn about
them.
Benefits of this approach: we'd have a consistent UI which didn't
require too much knowledge of the internals of Git, and all of the
following would work:
git diff --no-pager args...
git-diff --no-pager args...
git --no-pager diff args...
Looking at the options currently parsed by git.c, I think most of them
would be very straightforward to support in the way described above:
--paginate
--no-pager
--git-dir
--work-tree
--bare
Existing special cases:
--help (calls "git help foo")
--version
Problematic:
-p (short form of --paginate: seeing as -p is already used to mean
other things by other commands, I think this should be deprecated as a
synonym for --paginate)
Tricky:
--exec-path (this is the only one which git.c *must* handle before
passing control to the "subcommand", so it actually has to look ahead
past the "subcommand" part in order to see it and act upon it.
Basically it would just have to look at all the arguments up to but
not including a "--" pathspec marker checking to see if --exec-path is
supplied)
Of course, the above plan will only work for builtins, not for
scripts. An additional step would be needed to enable scripts to
handle these options; perhaps teaching "git rev-parse" something...
Cheers,
Wincent
^ permalink raw reply
* Re: [PATCH 5/6] whitespace: more accurate initial-indent highlighting
From: Junio C Hamano @ 2007-12-17 8:04 UTC (permalink / raw)
To: Wincent Colaiuta; +Cc: J. Bruce Fields, Git Mailing List
In-Reply-To: <ACA0791E-189F-4E19-AE87-C7D1163C0366@wincent.com>
Wincent Colaiuta <win@wincent.com> writes:
> Basically I would have proposed extracting out each type of whitespace
> error into an inline function in ws.c, where it could be used by both
> check_and_emit_line() in ws.c and apply_one_fragment() in builtin-
> apply.c.
>
> Unfortunately, mixing checking and emission phases makes this proposed
> refactoring a little bit ugly.
The right refactoring would be what JBF hinted in his message, to record
and return a list of suspicious ranges from the checker function and
have the highlighter and the fixer make use of that list.
Such a refactoring is still possible but I think it is beyond the scope
of pre 1.5.4 clean-up.
^ permalink raw reply
* Re: [PATCH 5/6] whitespace: more accurate initial-indent highlighting
From: Wincent Colaiuta @ 2007-12-17 8:00 UTC (permalink / raw)
To: J. Bruce Fields; +Cc: Git Mailing List, Junio Hamano
In-Reply-To: <1197822702-5262-6-git-send-email-bfields@citi.umich.edu>
El 16/12/2007, a las 17:31, J. Bruce Fields escribió:
> Instead of highlighting the entire initial indent, highlight only the
> problematic spaces.
>
> In the case of an indent like ' \t \t' there may be multiple
> problematic
> ranges, so it's easiest to emit the highlighting as we go instead of
> trying rember disjoint ranges and do it all at the end.
I'm relatively opposed to mixing the "check" and the "emit" phases
here because it will make further refactoring harder.
In the initial version of your series you forgot that there was
another place in the codebase ("git apply --whitespace=fix") where
whitespace errors are detected, and so you introduced an inconsistency
which you later fixed up. To me this is indicative of the fact that we
need to refactor further so that there really is only *one* place
where the whitespace checking logic is implemented.
Basically I would have proposed extracting out each type of whitespace
error into an inline function in ws.c, where it could be used by both
check_and_emit_line() in ws.c and apply_one_fragment() in builtin-
apply.c.
Unfortunately, mixing checking and emission phases makes this proposed
refactoring a little bit ugly.
But I see it's already gone into master as ffe56885.
Wincent
^ permalink raw reply
* Re: [RFH] convert shortlog to use parse_options
From: Junio C Hamano @ 2007-12-17 7:59 UTC (permalink / raw)
To: Andreas Ericsson; +Cc: Pierre Habouzit, Jeff King, Kristian Høgsberg, git
In-Reply-To: <47662715.9070200@op5.se>
Andreas Ericsson <ae@op5.se> writes:
> Junio C Hamano wrote:
>
>> ... The
>> "must stick" restriction feels Ok on paper but in practice it looks
>> rather draconian and very user unfriendly.
>
> Usually, optional arguments warrant adding a second parameter. This can
> often even improve usability, as it's never unclear or ambiguous what's
> happening. For the 'git tag -l' case, I'd use something like
> 'git tag -l --match="regex"' or some such,...
That is essentially arguing for POSIXly correct "do not allow optional
option-arguments" (utility syntax guidelines #7). That position might
be politically correct, but I am already discussing beyond that:
usability.
For "git tag -l", the fix was rather simple, as the option would either
have taken a zero pattern (list all) or a single pattern (list matching
this pattern), and the command itself did not take any extra arguments,
so that was what I did in the patch. Compare your POSIXly correct
version:
git tag -l (ok)
git tag -l pattern (not ok)
git tag -l --match=pattern (ok)
with the traditional (and fixed):
git tag -l (ok)
git tag -l pattern (ok)
git tag -l pattern garbage (not ok)
Which one is easier for the user?
^ permalink raw reply
* Re: git gui blame utf-8 bugs
From: Finn Arne Gangstad @ 2007-12-17 7:50 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Jakub Narebski, git
In-Reply-To: <m3k5nhqwkn.fsf@roke.D-201>
On Fri, Dec 14, 2007 at 04:39:59AM -0800, Jakub Narebski wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
> > Finn Arne Gangstad <finnag@pvv.org> wrote:
> > > git gui has some utf-8 bugs:
> >
> > It has several. :-)
> >
> > > If you do git gui blame <file>, and the file contains utf-8 text,
> > > the lines are not parsed as utf-8, but seemingly as iso-8859-1 instead.
> >
> > Right. git-gui is keying off the environment setting for LANG, so I
> > guess its set to iso-8859-1 on your system but you are working with a
> > [...]
Setting LANG does not seem to have any effect at all (neiher for file
contents, aurhor or commit messages).
LANG=en_US.UTF-8 git gui blame dir.c -> same
LANG=utf-8 git gui blame dir.c -> same
- Finn Arne
^ permalink raw reply
* Re: [PATCH v2] threaded pack-objects: Use condition variables for thread communication.
From: Johannes Sixt @ 2007-12-17 7:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Peter Baumann, git, Nicolas Pitre
In-Reply-To: <200712162045.34456.johannes.sixt@telecom.at>
I just discovered a theoretical resource leakage:
> + /* Partition the work amongst work threads. */
> for (i = 0; i < delta_search_threads; i++) {
...
> + pthread_mutex_init(&p[i].mutex, NULL);
> + pthread_cond_init(&p[i].cond, NULL);
These two initializations...
> }
>
> + /* Start work threads. */
> + for (i = 0; i < delta_search_threads; i++) {
> + if (!p[i].list_size)
> + continue;
... should go here because...
> + ret = pthread_create(&p[i].thread, NULL,
> + threaded_find_deltas, &p[i]);
> + if (ret)
> + die("unable to create thread: %s", strerror(ret));
> + active_threads++;
> + }
...
> + while (active_threads) {
...
> if (!sub_size) {
> pthread_join(target->thread, NULL);
> + pthread_cond_destroy(&target->cond);
> + pthread_mutex_destroy(&target->mutex);
... we tear down only those for which we actually started a thread.
> active_threads--;
> }
> + }
Will send a patch this evening.
-- Hannes
^ permalink raw reply
* Re: [RFH] convert shortlog to use parse_options
From: Andreas Ericsson @ 2007-12-17 7:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pierre Habouzit, Jeff King, Kristian Høgsberg, git
In-Reply-To: <7vir2xeq76.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
>
>> Yep, let's do it then. Note that's the reason why I felt we need a
>> manual page about this, because we should give some guidelines of what
>> is safe for scripting.
>
> There are some fallouts from the series, though. I've fixed up git-tag
> but I strongly suspect all of parseopt users now need careful auditing.
> If we cannot be confident with the parseoptified commands within
> reasonably short timeframe by -rc1, we may end up having to revert the
> parseopt changes from them, which I'd rather avoid, but if you look at
> the git-tag change (especially for -l) you would understand it. The
> "must stick" restriction feels Ok on paper but in practice it looks
> rather draconian and very user unfriendly.
>
Usually, optional arguments warrant adding a second parameter. This can
often even improve usability, as it's never unclear or ambiguous what's
happening. For the 'git tag -l' case, I'd use something like
'git tag -l --match="regex"' or some such, or perhaps make "-l" its own
subcommand ("list") with a built-in alias of -l. That means "-l" has to
be the first argument after "git tag" on the command-line, but I suspect
it doesn't make much sense to use it along with other options anyway, so
perhaps that's not much of an issue.
--
Andreas Ericsson andreas.ericsson@op5.se
OP5 AB www.op5.se
Tel: +46 8-230225 Fax: +46 8-230231
^ permalink raw reply
* [PATCH] (squashme) gitcli documentation fixups
From: Junio C Hamano @ 2007-12-17 7:28 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Jeff King, git
In-Reply-To: <20071213102724.GE12398@artemis.madism.org>
This comes directly on top of gitcli documentation patch and is intended
to be squashed into it.
I looked around to see if there is a good place to add gitlink:gitcli[5]
but unfortunately I did not find any. One possibility, once this
document is enhanced enough to be usable as "introduction to scripting
using plumbing", we could add it to near the top of git(7) where we
refer to the tutorial, user manual and the everyday document, but in the
current form it is too sketchy and does not cover enough. But we have
to start somewhere.
I think we should add the first rule in the bulletted list.
* avoid reinventing the wheel.
but it needs a bit more explanation. Quite a few people seem to try to
reinvent "git rev-parse --verify HEAD" in their scripts using much
higher level "git show -s -1 --pretty=format:xxx", which is unfortunate
and disgusting at the same time.
---
Documentation/gitcli.txt | 45 +++++++++++++++++++++++++++------------------
Makefile | 1 +
2 files changed, 28 insertions(+), 18 deletions(-)
diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
index 3b3f5e4..f790b78 100644
--- a/Documentation/gitcli.txt
+++ b/Documentation/gitcli.txt
@@ -3,7 +3,7 @@ gitcli(5)
NAME
----
-gitcli - git command line interface and its usual conventions
+gitcli - git command line interface and conventions
SYNOPSIS
--------
@@ -12,31 +12,40 @@ gitcli
DESCRIPTION
-----------
-This manual intends to describe best practice in how to use git CLI. Here are
+
+This manual describes best practice in how to use git CLI. Here are
the rules that you should follow when you are scripting git:
* it's preferred to use the non dashed form of git commands, which means that
you should prefer `"git foo"` to `"git-foo"`.
- * splitting short option switches in separate atoms (prefer `"git foo -a -b"`
+ * splitting short options to separate words (prefer `"git foo -a -b"`
to `"git foo -ab"`, the latter may not even work).
- * when a command line switch takes an argument, use the 'sticked' form, which
- means that you must prefer `"git foo -oArg"` to `"git foo -o Arg"` for short
- option switches, and `"git foo --long-opt=Arg"` to `"git foo --long-opt Arg"`
- for long switches.
+ * when a command line option takes an argument, use the 'sticked' form. In
+ other words, write `"git foo -oArg"` instead of `"git foo -o Arg"` for short
+ options, and `"git foo --long-opt=Arg"` instead of `"git foo --long-opt Arg"`
+ for long options. An option that takes optional option-argument must be
+ written in the 'sticked' form.
+
+ * when you give a revision parameter to a command, make sure the parameter is
+ not ambiguous with a name of a file in the work tree. E.g. do not write
+ `"git log -1 HEAD"` but write `"git log -1 HEAD --"`; the former will not work
+ if you happen to have a file called `HEAD` in the work tree.
ENHANCED CLI
------------
-From the git 1.5.4 series and further, git commands (not all of them at the
-time of the writing though) come with an enhanced option parser with nice
-facilities. Here is an exhaustive list of them
+From the git 1.5.4 series and further, many git commands (not all of them at the
+time of the writing though) come with an enhanced option parser.
+
+Here is an exhaustive list of the facilities provided by this option parser.
+
Magic Options
~~~~~~~~~~~~~
Commands which have the enhanced option parser activated all understand a
-couple of magic command line switches:
+couple of magic command line options:
-h::
gives a pretty printed usage of the command.
@@ -54,14 +63,14 @@ usage: git-describe [options] <committish>*
---------------------------------------------
--help-all::
- Some git commands takes options that are only used for plumbing or that
+ Some git commands take options that are only used for plumbing or that
are deprecated, and such options are hidden from the default usage. This
- switch gives the full list of options.
+ option gives the full list of options.
Negating options
~~~~~~~~~~~~~~~~
-Another things to keep in mind is that long options can be negated. For
+Boolean options with long option names can be negated by prefixing `"--no-"`. For
example, `"git branch"` has the option `"--track"` which is 'on' by default. You
can use `"--no-track"` to override that behaviour. The same goes for `"--color"`
and `"--no-color"`.
@@ -74,10 +83,10 @@ options. This means that you can for example use `"git rm -rf"` or
`"git clean -fdx"`.
-Separating argument from the switch
+Separating argument from the option
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-Also for option switches that take a mandatory argument, you can separate it
-from the switch. That means that all the following uses are correct:
+You can write the mandatory option parameter to an option as a separate
+word on the command line. That means that all the following uses work:
----------------------------
$ git foo --long-opt=Arg
@@ -86,7 +95,7 @@ $ git foo -oArg
$ git foo -o Arg
----------------------------
-However, this is *NOT* possible for switches with an optionnal value, where the
+However, this is *NOT* allowed for switches with an optionnal value, where the
'sticked' form must be used:
----------------------------
$ git describe --abbrev HEAD # correct
diff --git a/Makefile b/Makefile
index 617e5f5..fd9b939 100644
--- a/Makefile
+++ b/Makefile
@@ -1172,6 +1172,7 @@ check-docs::
documented,gitattributes | \
documented,gitignore | \
documented,gitmodules | \
+ documented,gitcli | \
documented,git-tools | \
sentinel,not,matching,is,ok ) continue ;; \
esac; \
--
1.5.4.rc0.54.g3eb2
^ permalink raw reply related
* [PATCH] builtin-tag: fix fallouts from recent parsopt restriction.
From: Junio C Hamano @ 2007-12-17 7:28 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Jeff King, git
In-Reply-To: <20071213102636.GD12398@artemis.madism.org>
The command freely used optional option-argments for its -l and -n options.
I think allowing "git tag -n xxx" without barfing was an error to begin with,
but not supporting "git tag -l pattern" form is a serious regression.
So this fixes the handling of -l to reinstate the original behaviour while
detecting a user error "git tag -l pattern garbage", and adjusts tests that
use "-n param" form to use "-nparam".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-tag.c | 15 +++++++-
t/t7004-tag.sh | 110 +++++++++++++++++++++++++------------------------------
2 files changed, 63 insertions(+), 62 deletions(-)
diff --git a/builtin-tag.c b/builtin-tag.c
index 274901a..5213c62 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -371,7 +371,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
int annotate = 0, sign = 0, force = 0, lines = 0,
delete = 0, verify = 0;
- char *list = NULL, *msgfile = NULL, *keyid = NULL;
+ const char *list = NULL, *msgfile = NULL, *keyid = NULL;
const char *no_pattern = "NO_PATTERN";
struct msg_arg msg = { 0, STRBUF_INIT };
struct option options[] = {
@@ -407,8 +407,19 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (sign)
annotate = 1;
- if (list)
+ if (list) {
+ /*
+ * This is unfortunate but requiring "git tag -lpattern" and not
+ * allowing "git tag -l pattern" is a serious regression.
+ */
+ if (argc && list == no_pattern) {
+ list = argv[0];
+ argc--;
+ }
+ if (argc)
+ die("extra argument after -l[pattern]: %s", argv[0]);
return list_tags(list == no_pattern ? NULL : list, lines);
+ }
if (delete)
return for_each_tag_name(argv, delete_tag);
if (verify)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 09d56e0..7f58038 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -491,25 +491,19 @@ test_expect_success \
echo "tag-one-line" >expect &&
git-tag -l | grep "^tag-one-line" >actual &&
git diff expect actual &&
- git-tag -n 0 -l | grep "^tag-one-line" >actual &&
+ git-tag -n0 -l | grep "^tag-one-line" >actual &&
git diff expect actual &&
- git-tag -n 0 -l tag-one-line >actual &&
+ git-tag -n0 -l tag-one-line >actual &&
git diff expect actual &&
echo "tag-one-line A msg" >expect &&
- git-tag -n xxx -l | grep "^tag-one-line" >actual &&
- git diff expect actual &&
- git-tag -n "" -l | grep "^tag-one-line" >actual &&
- git diff expect actual &&
- git-tag -n 1 -l | grep "^tag-one-line" >actual &&
- git diff expect actual &&
git-tag -n -l | grep "^tag-one-line" >actual &&
git diff expect actual &&
- git-tag -n 1 -l tag-one-line >actual &&
+ git-tag -n1 -l | grep "^tag-one-line" >actual &&
git diff expect actual &&
- git-tag -n 2 -l tag-one-line >actual &&
+ git-tag -n2 -l tag-one-line >actual &&
git diff expect actual &&
- git-tag -n 999 -l tag-one-line >actual &&
+ git-tag -n999 -l tag-one-line >actual &&
git diff expect actual
'
@@ -520,21 +514,21 @@ test_expect_success \
echo "tag-zero-lines" >expect &&
git-tag -l | grep "^tag-zero-lines" >actual &&
git diff expect actual &&
- git-tag -n 0 -l | grep "^tag-zero-lines" >actual &&
+ git-tag -n0 -l | grep "^tag-zero-lines" >actual &&
git diff expect actual &&
- git-tag -n 0 -l tag-zero-lines >actual &&
+ git-tag -n0 -l tag-zero-lines >actual &&
git diff expect actual &&
echo "tag-zero-lines " >expect &&
- git-tag -n 1 -l | grep "^tag-zero-lines" >actual &&
+ git-tag -n1 -l | grep "^tag-zero-lines" >actual &&
git diff expect actual &&
git-tag -n -l | grep "^tag-zero-lines" >actual &&
git diff expect actual &&
- git-tag -n 1 -l tag-zero-lines >actual &&
+ git-tag -n1 -l tag-zero-lines >actual &&
git diff expect actual &&
- git-tag -n 2 -l tag-zero-lines >actual &&
+ git-tag -n2 -l tag-zero-lines >actual &&
git diff expect actual &&
- git-tag -n 999 -l tag-zero-lines >actual &&
+ git-tag -n999 -l tag-zero-lines >actual &&
git diff expect actual
'
@@ -548,37 +542,37 @@ test_expect_success \
echo "tag-lines" >expect &&
git-tag -l | grep "^tag-lines" >actual &&
git diff expect actual &&
- git-tag -n 0 -l | grep "^tag-lines" >actual &&
+ git-tag -n0 -l | grep "^tag-lines" >actual &&
git diff expect actual &&
- git-tag -n 0 -l tag-lines >actual &&
+ git-tag -n0 -l tag-lines >actual &&
git diff expect actual &&
echo "tag-lines tag line one" >expect &&
- git-tag -n 1 -l | grep "^tag-lines" >actual &&
+ git-tag -n1 -l | grep "^tag-lines" >actual &&
git diff expect actual &&
git-tag -n -l | grep "^tag-lines" >actual &&
git diff expect actual &&
- git-tag -n 1 -l tag-lines >actual &&
+ git-tag -n1 -l tag-lines >actual &&
git diff expect actual &&
echo " tag line two" >>expect &&
- git-tag -n 2 -l | grep "^ *tag.line" >actual &&
+ git-tag -n2 -l | grep "^ *tag.line" >actual &&
git diff expect actual &&
- git-tag -n 2 -l tag-lines >actual &&
+ git-tag -n2 -l tag-lines >actual &&
git diff expect actual &&
echo " tag line three" >>expect &&
- git-tag -n 3 -l | grep "^ *tag.line" >actual &&
+ git-tag -n3 -l | grep "^ *tag.line" >actual &&
git diff expect actual &&
- git-tag -n 3 -l tag-lines >actual &&
+ git-tag -n3 -l tag-lines >actual &&
git diff expect actual &&
- git-tag -n 4 -l | grep "^ *tag.line" >actual &&
+ git-tag -n4 -l | grep "^ *tag.line" >actual &&
git diff expect actual &&
- git-tag -n 4 -l tag-lines >actual &&
+ git-tag -n4 -l tag-lines >actual &&
git diff expect actual &&
- git-tag -n 99 -l | grep "^ *tag.line" >actual &&
+ git-tag -n99 -l | grep "^ *tag.line" >actual &&
git diff expect actual &&
- git-tag -n 99 -l tag-lines >actual &&
+ git-tag -n99 -l tag-lines >actual &&
git diff expect actual
'
@@ -906,25 +900,21 @@ test_expect_success \
echo "stag-one-line" >expect &&
git-tag -l | grep "^stag-one-line" >actual &&
git diff expect actual &&
- git-tag -n 0 -l | grep "^stag-one-line" >actual &&
+ git-tag -n0 -l | grep "^stag-one-line" >actual &&
git diff expect actual &&
- git-tag -n 0 -l stag-one-line >actual &&
+ git-tag -n0 -l stag-one-line >actual &&
git diff expect actual &&
echo "stag-one-line A message line signed" >expect &&
- git-tag -n xxx -l | grep "^stag-one-line" >actual &&
- git diff expect actual &&
- git-tag -n "" -l | grep "^stag-one-line" >actual &&
- git diff expect actual &&
- git-tag -n 1 -l | grep "^stag-one-line" >actual &&
- git diff expect actual &&
git-tag -n -l | grep "^stag-one-line" >actual &&
git diff expect actual &&
- git-tag -n 1 -l stag-one-line >actual &&
+ git-tag -n1 -l | grep "^stag-one-line" >actual &&
git diff expect actual &&
- git-tag -n 2 -l stag-one-line >actual &&
+ git-tag -n1 -l stag-one-line >actual &&
git diff expect actual &&
- git-tag -n 999 -l stag-one-line >actual &&
+ git-tag -n2 -l stag-one-line >actual &&
+ git diff expect actual &&
+ git-tag -n999 -l stag-one-line >actual &&
git diff expect actual
'
@@ -935,21 +925,21 @@ test_expect_success \
echo "stag-zero-lines" >expect &&
git-tag -l | grep "^stag-zero-lines" >actual &&
git diff expect actual &&
- git-tag -n 0 -l | grep "^stag-zero-lines" >actual &&
+ git-tag -n0 -l | grep "^stag-zero-lines" >actual &&
git diff expect actual &&
- git-tag -n 0 -l stag-zero-lines >actual &&
+ git-tag -n0 -l stag-zero-lines >actual &&
git diff expect actual &&
echo "stag-zero-lines " >expect &&
- git-tag -n 1 -l | grep "^stag-zero-lines" >actual &&
- git diff expect actual &&
git-tag -n -l | grep "^stag-zero-lines" >actual &&
git diff expect actual &&
- git-tag -n 1 -l stag-zero-lines >actual &&
+ git-tag -n1 -l | grep "^stag-zero-lines" >actual &&
+ git diff expect actual &&
+ git-tag -n1 -l stag-zero-lines >actual &&
git diff expect actual &&
- git-tag -n 2 -l stag-zero-lines >actual &&
+ git-tag -n2 -l stag-zero-lines >actual &&
git diff expect actual &&
- git-tag -n 999 -l stag-zero-lines >actual &&
+ git-tag -n999 -l stag-zero-lines >actual &&
git diff expect actual
'
@@ -963,37 +953,37 @@ test_expect_success \
echo "stag-lines" >expect &&
git-tag -l | grep "^stag-lines" >actual &&
git diff expect actual &&
- git-tag -n 0 -l | grep "^stag-lines" >actual &&
+ git-tag -n0 -l | grep "^stag-lines" >actual &&
git diff expect actual &&
- git-tag -n 0 -l stag-lines >actual &&
+ git-tag -n0 -l stag-lines >actual &&
git diff expect actual &&
echo "stag-lines stag line one" >expect &&
- git-tag -n 1 -l | grep "^stag-lines" >actual &&
- git diff expect actual &&
git-tag -n -l | grep "^stag-lines" >actual &&
git diff expect actual &&
- git-tag -n 1 -l stag-lines >actual &&
+ git-tag -n1 -l | grep "^stag-lines" >actual &&
+ git diff expect actual &&
+ git-tag -n1 -l stag-lines >actual &&
git diff expect actual &&
echo " stag line two" >>expect &&
- git-tag -n 2 -l | grep "^ *stag.line" >actual &&
+ git-tag -n2 -l | grep "^ *stag.line" >actual &&
git diff expect actual &&
- git-tag -n 2 -l stag-lines >actual &&
+ git-tag -n2 -l stag-lines >actual &&
git diff expect actual &&
echo " stag line three" >>expect &&
- git-tag -n 3 -l | grep "^ *stag.line" >actual &&
+ git-tag -n3 -l | grep "^ *stag.line" >actual &&
git diff expect actual &&
- git-tag -n 3 -l stag-lines >actual &&
+ git-tag -n3 -l stag-lines >actual &&
git diff expect actual &&
- git-tag -n 4 -l | grep "^ *stag.line" >actual &&
+ git-tag -n4 -l | grep "^ *stag.line" >actual &&
git diff expect actual &&
- git-tag -n 4 -l stag-lines >actual &&
+ git-tag -n4 -l stag-lines >actual &&
git diff expect actual &&
- git-tag -n 99 -l | grep "^ *stag.line" >actual &&
+ git-tag -n99 -l | grep "^ *stag.line" >actual &&
git diff expect actual &&
- git-tag -n 99 -l stag-lines >actual &&
+ git-tag -n99 -l stag-lines >actual &&
git diff expect actual
'
--
1.5.4.rc0.54.g3eb2
^ permalink raw reply related
* Re: [RFH] convert shortlog to use parse_options
From: Junio C Hamano @ 2007-12-17 7:27 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Jeff King, Kristian Høgsberg, git
In-Reply-To: <20071215110311.GA11321@artemis.madism.org>
Pierre Habouzit <madcoder@debian.org> writes:
> Yep, let's do it then. Note that's the reason why I felt we need a
> manual page about this, because we should give some guidelines of what
> is safe for scripting.
There are some fallouts from the series, though. I've fixed up git-tag
but I strongly suspect all of parseopt users now need careful auditing.
If we cannot be confident with the parseoptified commands within
reasonably short timeframe by -rc1, we may end up having to revert the
parseopt changes from them, which I'd rather avoid, but if you look at
the git-tag change (especially for -l) you would understand it. The
"must stick" restriction feels Ok on paper but in practice it looks
rather draconian and very user unfriendly.
I'll be sending out two patches shortly. The result will sit in 'pu'
waiting for an Ack and/or improvement, but that will happen only when I
got around pushing things out, which may come later than you see the
patches.
^ permalink raw reply
* Re: [PATCH] threaded pack-objects: Use condition variables for thread communication.
From: Dmitry Potapov @ 2007-12-17 4:26 UTC (permalink / raw)
To: Peter Baumann; +Cc: Johannes Sixt, git, Nicolas Pitre, Junio C Hamano
In-Reply-To: <20071216190016.GC4999@xp.machine.xx>
On Sun, Dec 16, 2007 at 08:00:16PM +0100, Peter Baumann wrote:
> On Sun, Dec 16, 2007 at 07:41:37PM +0100, Johannes Sixt wrote:
> > On Sunday 16 December 2007 13:05, Peter Baumann wrote:
> > > On Sun, Dec 16, 2007 at 12:18:53AM +0100, Johannes Sixt wrote:
> > > > +
> > > > + progress_lock();
> > > > + me->working = 0;
> > > > + progress_unlock();
> > > > + pthread_cond_signal(&progress_cond);
> > >
> > > Shouldn't the pthread_cond_signal be inside the lock?
> > > e.g. swap progress_unlock() with pthread_cond_signal(&progress_cond)
> >
> > No, that's not necessary. Both ways are correct, but if it's outside the lock
> > there is less contention on the mutex (because the waiting thread must
> > acquire the mutex lock before it can return from pthread_cond_wait).
> >
>
> At least I was told otherwise and [1] backs my knowledge up. Are you
> really sure?
>
> -Peter
>
> http://docs.sun.com/app/docs/doc/806-5257/6je9h032r?a=view#sync-53686
The POSIX standard clearly says that usage of pthread_cond_signal ouside
of the mutex protection is allowed:
===
The pthread_cond_signal() or pthread_cond_broadcast() functions may be
called by a thread whether or not it currently owns the mutex that
threads calling pthread_cond_wait() or pthread_cond_timedwait() have
associated with the condition variable during their waits; however, if
predictable scheduling behaviour is required, then that mutex is locked
by the thread calling pthread_cond_signal() or pthread_cond_broadcast().
===
http://www.opengroup.org/onlinepubs/007908775/xsh/pthread_cond_signal.html
And the argumentation provided by Sun's manual:
> Otherwise, the condition variable could be signaled between the test
> of the associated condition and blocking in pthread_cond_wait(), which
> can cause an infinite wait.
sounds strange to me. Indeed the condition variable could be signaled
between the test and blocking, but the condition itself is false, so the
thread should be blocked anyway. The only effect of signalling outside
of the mutex protection is that the thread blocked on the condition
variable can be waken up while the associated condition is still false,
but it is not a problem.
As to what is more optimal, it is less clear. Because on one side,
signaling under the lock increases contention, but, on the other hand,
it avoids spurious wakeup. So, I suppose it could be implementation
specific.
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] builtin-commit: make summary output consistent with status
From: Daniel Barkalow @ 2007-12-17 3:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwsregrom.fsf@gitster.siamese.dyndns.org>
On Sun, 16 Dec 2007, Junio C Hamano wrote:
> * And this fixes the "7 files" because print_summary() does not pass -B
> -M which makes it inconsistent with the status output. After this
> you should see "6 files".
Yup, it now looks like what I thought that commit consisted of.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH v2] threaded pack-objects: Use condition variables for thread communication.
From: Nicolas Pitre @ 2007-12-17 3:13 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Peter Baumann, git, Junio C Hamano
In-Reply-To: <200712162045.34456.johannes.sixt@telecom.at>
On Sun, 16 Dec 2007, Johannes Sixt wrote:
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
One micro nit:
> + for (;;) {
> + for (i = 0; !target && i < delta_search_threads; i++)
> + if (!p[i].working)
> + target = &p[i];
> + if (target)
> + break;
> + pthread_cond_wait(&progress_cond, &progress_mutex);
> + };
There's a spurious ; here.
Otherwise:
Acked-by: Nicolas Pitre <nico@cam.org>
Nicolas
^ permalink raw reply
* Re: end of file newline regression
From: Junio C Hamano @ 2007-12-17 1:24 UTC (permalink / raw)
To: David Miller; +Cc: git
In-Reply-To: <20071216.155413.165709543.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> I've stumbled into a problem with current mainline.
>
> I have this changeset that I can't get GIT to properly
> generate diffs for. It always give me:
>
> diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
> index 4440532..096974b 100644
> --- a/net/ipv6/esp6.c
> +++ b/net/ipv6/esp6.c
> @@ -177,5 +177,4 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
> BUG();
>
> if (unlikely(memcmp(esp->auth.work_icv, sum, alen))) {
> - x->stats.integrity_failed++;
> - ret = -EINVAL;
> \ No newline at end of file
> + ret = -EBADMSG;
> \ No newline at end of file
>
> No matter what I do, 1.5.3.7 doesn't do this.
Sorry, there was a recent brown-paper-bag. Should have been fixed with
the patches flew on the list for the past 24 hours or so. Could you try
it with the tip of 'master'?
^ permalink raw reply
* end of file newline regression
From: David Miller @ 2007-12-16 23:54 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: Text/Plain, Size: 902 bytes --]
I've stumbled into a problem with current mainline.
I have this changeset that I can't get GIT to properly
generate diffs for. It always give me:
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 4440532..096974b 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -177,5 +177,4 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
BUG();
if (unlikely(memcmp(esp->auth.work_icv, sum, alen))) {
- x->stats.integrity_failed++;
- ret = -EINVAL;
\ No newline at end of file
+ ret = -EBADMSG;
\ No newline at end of file
No matter what I do, 1.5.3.7 doesn't do this.
The file does have a newline at the end.
Here are files you can hopefully use to reproduce:
1) esp6.c
2) the patch, one that can be applied properly
You should be able to put that esp6.c into an empty
git repo, apply the patch in #2, then run "git diff >diff"
using mainline to see the problem.
[-- Attachment #2: esp6.c --]
[-- Type: Text/Plain, Size: 10795 bytes --]
/*
* Copyright (C)2002 USAGI/WIDE Project
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, write to the Free Software
* Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
*
* Authors
*
* Mitsuru KANDA @USAGI : IPv6 Support
* Kazunori MIYAZAWA @USAGI :
* Kunihiro Ishiguro <kunihiro@ipinfusion.com>
*
* This file is derived from net/ipv4/esp.c
*/
#include <linux/err.h>
#include <linux/module.h>
#include <net/ip.h>
#include <net/xfrm.h>
#include <net/esp.h>
#include <linux/scatterlist.h>
#include <linux/crypto.h>
#include <linux/kernel.h>
#include <linux/pfkeyv2.h>
#include <linux/random.h>
#include <linux/spinlock.h>
#include <net/icmp.h>
#include <net/ipv6.h>
#include <net/protocol.h>
#include <linux/icmpv6.h>
static int esp6_output(struct xfrm_state *x, struct sk_buff *skb)
{
int err;
struct ip_esp_hdr *esph;
struct crypto_blkcipher *tfm;
struct blkcipher_desc desc;
struct sk_buff *trailer;
int blksize;
int clen;
int alen;
int nfrags;
u8 *tail;
struct esp_data *esp = x->data;
/* skb is pure payload to encrypt */
err = -ENOMEM;
/* Round to block size */
clen = skb->len;
alen = esp->auth.icv_trunc_len;
tfm = esp->conf.tfm;
desc.tfm = tfm;
desc.flags = 0;
blksize = ALIGN(crypto_blkcipher_blocksize(tfm), 4);
clen = ALIGN(clen + 2, blksize);
if (esp->conf.padlen)
clen = ALIGN(clen, esp->conf.padlen);
if ((nfrags = skb_cow_data(skb, clen-skb->len+alen, &trailer)) < 0) {
goto error;
}
/* Fill padding... */
tail = skb_tail_pointer(trailer);
do {
int i;
for (i=0; i<clen-skb->len - 2; i++)
tail[i] = i + 1;
} while (0);
tail[clen-skb->len - 2] = (clen - skb->len) - 2;
pskb_put(skb, trailer, clen - skb->len);
skb_push(skb, -skb_network_offset(skb));
esph = ip_esp_hdr(skb);
*(skb_tail_pointer(trailer) - 1) = *skb_mac_header(skb);
*skb_mac_header(skb) = IPPROTO_ESP;
esph->spi = x->id.spi;
esph->seq_no = htonl(XFRM_SKB_CB(skb)->seq);
spin_lock_bh(&x->lock);
if (esp->conf.ivlen) {
if (unlikely(!esp->conf.ivinitted)) {
get_random_bytes(esp->conf.ivec, esp->conf.ivlen);
esp->conf.ivinitted = 1;
}
crypto_blkcipher_set_iv(tfm, esp->conf.ivec, esp->conf.ivlen);
}
do {
struct scatterlist *sg = &esp->sgbuf[0];
if (unlikely(nfrags > ESP_NUM_FAST_SG)) {
sg = kmalloc(sizeof(struct scatterlist)*nfrags, GFP_ATOMIC);
if (!sg)
goto unlock;
}
sg_init_table(sg, nfrags);
skb_to_sgvec(skb, sg,
esph->enc_data +
esp->conf.ivlen -
skb->data, clen);
err = crypto_blkcipher_encrypt(&desc, sg, sg, clen);
if (unlikely(sg != &esp->sgbuf[0]))
kfree(sg);
} while (0);
if (unlikely(err))
goto unlock;
if (esp->conf.ivlen) {
memcpy(esph->enc_data, esp->conf.ivec, esp->conf.ivlen);
crypto_blkcipher_get_iv(tfm, esp->conf.ivec, esp->conf.ivlen);
}
if (esp->auth.icv_full_len) {
err = esp_mac_digest(esp, skb, (u8 *)esph - skb->data,
sizeof(*esph) + esp->conf.ivlen + clen);
memcpy(pskb_put(skb, trailer, alen), esp->auth.work_icv, alen);
}
unlock:
spin_unlock_bh(&x->lock);
error:
return err;
}
static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
{
struct ipv6hdr *iph;
struct ip_esp_hdr *esph;
struct esp_data *esp = x->data;
struct crypto_blkcipher *tfm = esp->conf.tfm;
struct blkcipher_desc desc = { .tfm = tfm };
struct sk_buff *trailer;
int blksize = ALIGN(crypto_blkcipher_blocksize(tfm), 4);
int alen = esp->auth.icv_trunc_len;
int elen = skb->len - sizeof(*esph) - esp->conf.ivlen - alen;
int hdr_len = skb_network_header_len(skb);
int nfrags;
int ret = 0;
if (!pskb_may_pull(skb, sizeof(*esph))) {
ret = -EINVAL;
goto out;
}
if (elen <= 0 || (elen & (blksize-1))) {
ret = -EINVAL;
goto out;
}
/* If integrity check is required, do this. */
if (esp->auth.icv_full_len) {
u8 sum[alen];
ret = esp_mac_digest(esp, skb, 0, skb->len - alen);
if (ret)
goto out;
if (skb_copy_bits(skb, skb->len - alen, sum, alen))
BUG();
if (unlikely(memcmp(esp->auth.work_icv, sum, alen))) {
x->stats.integrity_failed++;
ret = -EINVAL;
goto out;
}
}
if ((nfrags = skb_cow_data(skb, 0, &trailer)) < 0) {
ret = -EINVAL;
goto out;
}
skb->ip_summed = CHECKSUM_NONE;
esph = (struct ip_esp_hdr *)skb->data;
iph = ipv6_hdr(skb);
/* Get ivec. This can be wrong, check against another impls. */
if (esp->conf.ivlen)
crypto_blkcipher_set_iv(tfm, esph->enc_data, esp->conf.ivlen);
{
u8 nexthdr[2];
struct scatterlist *sg = &esp->sgbuf[0];
u8 padlen;
if (unlikely(nfrags > ESP_NUM_FAST_SG)) {
sg = kmalloc(sizeof(struct scatterlist)*nfrags, GFP_ATOMIC);
if (!sg) {
ret = -ENOMEM;
goto out;
}
}
sg_init_table(sg, nfrags);
skb_to_sgvec(skb, sg,
sizeof(*esph) + esp->conf.ivlen,
elen);
ret = crypto_blkcipher_decrypt(&desc, sg, sg, elen);
if (unlikely(sg != &esp->sgbuf[0]))
kfree(sg);
if (unlikely(ret))
goto out;
if (skb_copy_bits(skb, skb->len-alen-2, nexthdr, 2))
BUG();
padlen = nexthdr[0];
if (padlen+2 >= elen) {
LIMIT_NETDEBUG(KERN_WARNING "ipsec esp packet is garbage padlen=%d, elen=%d\n", padlen+2, elen);
ret = -EINVAL;
goto out;
}
/* ... check padding bits here. Silly. :-) */
/* RFC4303: Drop dummy packets without any error */
if (nexthdr[1] == IPPROTO_NONE) {
ret = -EINVAL;
goto out;
}
pskb_trim(skb, skb->len - alen - padlen - 2);
ret = nexthdr[1];
}
__skb_pull(skb, sizeof(*esph) + esp->conf.ivlen);
skb_set_transport_header(skb, -hdr_len);
out:
return ret;
}
static u32 esp6_get_mtu(struct xfrm_state *x, int mtu)
{
struct esp_data *esp = x->data;
u32 blksize = ALIGN(crypto_blkcipher_blocksize(esp->conf.tfm), 4);
u32 align = max_t(u32, blksize, esp->conf.padlen);
u32 rem;
mtu -= x->props.header_len + esp->auth.icv_trunc_len;
rem = mtu & (align - 1);
mtu &= ~(align - 1);
if (x->props.mode != XFRM_MODE_TUNNEL) {
u32 padsize = ((blksize - 1) & 7) + 1;
mtu -= blksize - padsize;
mtu += min_t(u32, blksize - padsize, rem);
}
return mtu - 2;
}
static void esp6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
int type, int code, int offset, __be32 info)
{
struct ipv6hdr *iph = (struct ipv6hdr*)skb->data;
struct ip_esp_hdr *esph = (struct ip_esp_hdr *)(skb->data + offset);
struct xfrm_state *x;
if (type != ICMPV6_DEST_UNREACH &&
type != ICMPV6_PKT_TOOBIG)
return;
x = xfrm_state_lookup((xfrm_address_t *)&iph->daddr, esph->spi, IPPROTO_ESP, AF_INET6);
if (!x)
return;
printk(KERN_DEBUG "pmtu discovery on SA ESP/%08x/" NIP6_FMT "\n",
ntohl(esph->spi), NIP6(iph->daddr));
xfrm_state_put(x);
}
static void esp6_destroy(struct xfrm_state *x)
{
struct esp_data *esp = x->data;
if (!esp)
return;
crypto_free_blkcipher(esp->conf.tfm);
esp->conf.tfm = NULL;
kfree(esp->conf.ivec);
esp->conf.ivec = NULL;
crypto_free_hash(esp->auth.tfm);
esp->auth.tfm = NULL;
kfree(esp->auth.work_icv);
esp->auth.work_icv = NULL;
kfree(esp);
}
static int esp6_init_state(struct xfrm_state *x)
{
struct esp_data *esp = NULL;
struct crypto_blkcipher *tfm;
if (x->ealg == NULL)
goto error;
if (x->encap)
goto error;
esp = kzalloc(sizeof(*esp), GFP_KERNEL);
if (esp == NULL)
return -ENOMEM;
if (x->aalg) {
struct xfrm_algo_desc *aalg_desc;
struct crypto_hash *hash;
hash = crypto_alloc_hash(x->aalg->alg_name, 0,
CRYPTO_ALG_ASYNC);
if (IS_ERR(hash))
goto error;
esp->auth.tfm = hash;
if (crypto_hash_setkey(hash, x->aalg->alg_key,
(x->aalg->alg_key_len + 7) / 8))
goto error;
aalg_desc = xfrm_aalg_get_byname(x->aalg->alg_name, 0);
BUG_ON(!aalg_desc);
if (aalg_desc->uinfo.auth.icv_fullbits/8 !=
crypto_hash_digestsize(hash)) {
NETDEBUG(KERN_INFO "ESP: %s digestsize %u != %hu\n",
x->aalg->alg_name,
crypto_hash_digestsize(hash),
aalg_desc->uinfo.auth.icv_fullbits/8);
goto error;
}
esp->auth.icv_full_len = aalg_desc->uinfo.auth.icv_fullbits/8;
esp->auth.icv_trunc_len = aalg_desc->uinfo.auth.icv_truncbits/8;
esp->auth.work_icv = kmalloc(esp->auth.icv_full_len, GFP_KERNEL);
if (!esp->auth.work_icv)
goto error;
}
tfm = crypto_alloc_blkcipher(x->ealg->alg_name, 0, CRYPTO_ALG_ASYNC);
if (IS_ERR(tfm))
goto error;
esp->conf.tfm = tfm;
esp->conf.ivlen = crypto_blkcipher_ivsize(tfm);
esp->conf.padlen = 0;
if (esp->conf.ivlen) {
esp->conf.ivec = kmalloc(esp->conf.ivlen, GFP_KERNEL);
if (unlikely(esp->conf.ivec == NULL))
goto error;
esp->conf.ivinitted = 0;
}
if (crypto_blkcipher_setkey(tfm, x->ealg->alg_key,
(x->ealg->alg_key_len + 7) / 8))
goto error;
x->props.header_len = sizeof(struct ip_esp_hdr) + esp->conf.ivlen;
switch (x->props.mode) {
case XFRM_MODE_BEET:
case XFRM_MODE_TRANSPORT:
break;
case XFRM_MODE_TUNNEL:
x->props.header_len += sizeof(struct ipv6hdr);
break;
default:
goto error;
}
x->data = esp;
return 0;
error:
x->data = esp;
esp6_destroy(x);
x->data = NULL;
return -EINVAL;
}
static struct xfrm_type esp6_type =
{
.description = "ESP6",
.owner = THIS_MODULE,
.proto = IPPROTO_ESP,
.flags = XFRM_TYPE_REPLAY_PROT,
.init_state = esp6_init_state,
.destructor = esp6_destroy,
.get_mtu = esp6_get_mtu,
.input = esp6_input,
.output = esp6_output,
.hdr_offset = xfrm6_find_1stfragopt,
};
static struct inet6_protocol esp6_protocol = {
.handler = xfrm6_rcv,
.err_handler = esp6_err,
.flags = INET6_PROTO_NOPOLICY,
};
static int __init esp6_init(void)
{
if (xfrm_register_type(&esp6_type, AF_INET6) < 0) {
printk(KERN_INFO "ipv6 esp init: can't add xfrm type\n");
return -EAGAIN;
}
if (inet6_add_protocol(&esp6_protocol, IPPROTO_ESP) < 0) {
printk(KERN_INFO "ipv6 esp init: can't add protocol\n");
xfrm_unregister_type(&esp6_type, AF_INET6);
return -EAGAIN;
}
return 0;
}
static void __exit esp6_fini(void)
{
if (inet6_del_protocol(&esp6_protocol, IPPROTO_ESP) < 0)
printk(KERN_INFO "ipv6 esp close: can't remove protocol\n");
if (xfrm_unregister_type(&esp6_type, AF_INET6) < 0)
printk(KERN_INFO "ipv6 esp close: can't remove xfrm type\n");
}
module_init(esp6_init);
module_exit(esp6_fini);
MODULE_LICENSE("GPL");
MODULE_ALIAS_XFRM_TYPE(AF_INET6, XFRM_PROTO_ESP);
[-- Attachment #3: diff --]
[-- Type: Text/Plain, Size: 354 bytes --]
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 4440532..096974b 100644
--- a/esp6.c
+++ b/esp6.c
@@ -177,8 +177,7 @@ static int esp6_input(struct xfrm_state *x, struct sk_buff *skb)
BUG();
if (unlikely(memcmp(esp->auth.work_icv, sum, alen))) {
- x->stats.integrity_failed++;
- ret = -EINVAL;
+ ret = -EBADMSG;
goto out;
}
}
^ permalink raw reply related
* [PATCH 2/2] builtin-commit: make summary output consistent with status
From: Junio C Hamano @ 2007-12-16 23:12 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0712151507420.5349@iabervon.org>
This enables -B -M to the summary output after a commit is made so that
it is in line with what is shown in git-status and commit log template.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* And this fixes the "7 files" because print_summary() does not pass -B
-M which makes it inconsistent with the status output. After this
you should see "6 files".
builtin-commit.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/builtin-commit.c b/builtin-commit.c
index 61770ef..0a91013 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -662,6 +662,9 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
rev.show_root_diff = 1;
rev.commit_format = get_commit_format("format:%h: %s");
rev.always_show_header = 0;
+ rev.diffopt.detect_rename = 1;
+ rev.diffopt.rename_limit = 100;
+ rev.diffopt.break_opt = 0;
diff_setup_done(&rev.diffopt);
printf("Created %scommit ", initial_commit ? "initial " : "");
--
1.5.4.rc0.52.gb90ce
^ permalink raw reply related
* [PATCH 1/2] builtin-commit: fix summary output.
From: Junio C Hamano @ 2007-12-16 23:11 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0712151507420.5349@iabervon.org>
Because print_summary() forgot to call diff_setup_done() after futzing with
diff output options, it failed to activate recursive diff, which resulted in
an incorrect summary.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This fixes the "5 files" problem you saw with your commit, which was
totally bogus (it looked at "git diff-tree HEAD" output that was
non-recursive, discarding the change of tree t/ itself). Now it will
show "7 files" in line with your "git show".
builtin-commit.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/builtin-commit.c b/builtin-commit.c
index 518ebe0..61770ef 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -662,6 +662,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
rev.show_root_diff = 1;
rev.commit_format = get_commit_format("format:%h: %s");
rev.always_show_header = 0;
+ diff_setup_done(&rev.diffopt);
printf("Created %scommit ", initial_commit ? "initial " : "");
--
1.5.4.rc0.52.gb90ce
^ permalink raw reply related
* Re: Oddity in git commit summary
From: Junio C Hamano @ 2007-12-16 23:01 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: git
In-Reply-To: <Pine.LNX.4.64.0712151507420.5349@iabervon.org>
Daniel Barkalow <barkalow@iabervon.org> writes:
> I just noticed that I got a strange summary from git commit -a --amend
> with the current master. It said:
>
> Created commit f16cb29: Build in checkout
> 5 files changed, 482 insertions(+), 299 deletions(-)
> create mode 100644 builtin-checkout.c
> delete mode 100755 git-checkout.sh
>
> But git show --stat says:
> 7 files changed, 780 insertions(+), 306 deletions(-)
>
> And git show --stat -C says:
> 6 files changed, 482 insertions(+), 8 deletions(-)
Because "git status" has -B -M and your second one does not pass any, I
would not worry about the first two differences. But 5 vs 6 seems
funny.
print_summary() is broken.
diff --git a/builtin-commit.c b/builtin-commit.c
index 518ebe0..8435702 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -663,6 +665,8 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
rev.commit_format = get_commit_format("format:%h: %s");
rev.always_show_header = 0;
+ diff_setup_done(&rev.diffopt);
+
printf("Created %scommit ", initial_commit ? "initial " : "");
if (!log_tree_commit(&rev, commit)) {
^ permalink raw reply related
* Re: [PATCH] Re-re-re-fix common tail optimization
From: Jeff King @ 2007-12-16 22:29 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, git
In-Reply-To: <7vtzmii8io.fsf@gitster.siamese.dyndns.org>
On Sun, Dec 16, 2007 at 02:23:27PM -0800, Junio C Hamano wrote:
> Yeah, I amended it without adding another "re-" to the title ;-) The
> result has been already pushed out.
OK. Too late, but it has my ack. ;)
> > Aren't we using "git diff" for the second diff there nowadays?
>
> Some people seem to think that is a good idea, but I generally do not
> like using "git diff" between expect and actual (both untracked) inside
> tests. The last "diff" is about validating what git does and using "git
> diff" there would make the test meaningless when "git diff" itself is
> broken.
I think that is a valid concern. But ISTR that were some issues with
using GNU diff. Commit 5bd74506 mentions getting rid of the dependency
in all existing tests, but gives no reason.
-Peff
^ permalink raw reply
* Re: [PATCH] Re-re-re-fix common tail optimization
From: Junio C Hamano @ 2007-12-16 22:28 UTC (permalink / raw)
To: Jeff King; +Cc: Linus Torvalds, git
In-Reply-To: <7vtzmii8io.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>>> +test_expect_success 'diff -U0' '
>>> +
>>> + git diff -U0 | sed -e "/^index/d" -e "s/$z2047/Z/g" >actual &&
>>> + diff -u expect actual
>>
>> Aren't we using "git diff" for the second diff there nowadays?
>
> Some people seem to think that is a good idea, but I generally do not
> like using "git diff" between expect and actual (both untracked) inside
> tests. The last "diff" is about validating what git does and using "git
> diff" there would make the test meaningless when "git diff" itself is
> broken.
>
> This is especially so because comparison between untracked files is a
> bolted-on afterthought and I am least confident about among the
> codepaths in the whole "git diff" (it is not even my nor Linus's code).
Side note. The "confidence" I am talking about the above is not about
the correct-working of the current code. It seems to work fine.
It is about the fact it was bolted on rather than designed in from the
beginning---it is much likely to subtly break than other parts that are
much more integrated when you change seemingly unrelated thing like
git-dir discovery and rename detection.
IOW, the confidence is about the fixability/maintainability when
somebody breaks it.
^ permalink raw reply
* Re: Windows binaries for qgit 2.0
From: Marco Costalba @ 2007-12-16 22:26 UTC (permalink / raw)
To: Abdelrazak Younes; +Cc: git
In-Reply-To: <fk3153$rtb$1@ger.gmane.org>
On Dec 16, 2007 12:11 PM, Abdelrazak Younes <younes.a@free.fr> wrote:
>
> Actually you might prefer to just use the LyX dependencies package that
> we provide for Windows developers, it contains Qt. I paste here the
> relevant part of our 'INSTALL.Win32':
>
Thanks, I've tried it but without success because I need MSVC 2005
installed, and currently is not.
Anyhow for now I have produced a version with mingw that seems more or
less to work, when I have a bit of time I will install MSVC 2005 and
try if with that compiler is better...
Thanks
Marco
^ 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