* [PATCH v2 2/2] CodingGuidelines: Add note forbidding use of 'which' in shell scripts
From: Tim Henigan @ 2012-02-24 23:12 UTC (permalink / raw)
To: git, gitster; +Cc: tim.henigan
In-Reply-To: <1330125178-9194-1-git-send-email-tim.henigan@gmail.com>
During the code review of a recent patch, it was noted that shell scripts
must not use 'which'. The output of the command is not machine parseable
and its exit code is not reliable across platforms.
It is better to use 'type' to accomplish this task.
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
Updated to the documentation pattern recommended by Junio Hamano:
"If you want to do Z, use X not Y, because Y is broken ..."
I grepped through the code and found the 'type <command' is indeed used
in place of 'which' to test for the presence of commands on $PATH.
Documentation/CodingGuidelines | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 5a190b9..816c5ad 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -49,6 +49,11 @@ For shell scripts specifically (not exhaustive):
properly nests. It should have been the way Bourne spelled
it from day one, but unfortunately isn't.
+ - If you want to find out if a command is available on the user's
+ $PATH, you should use 'type <command>', instead of 'which'.
+ The output of 'which' is not machine parseable and its exit code
+ is not reliable across platforms.
+
- We use POSIX compliant parameter substitutions and avoid bashisms;
namely:
--
1.7.9.1
^ permalink raw reply related
* Re: [PATCH 01/11] Add more large blob test cases
From: Peter Baumann @ 2012-02-27 20:18 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1330329315-11407-2-git-send-email-pclouds@gmail.com>
A minor spelling error in the text.
On Mon, Feb 27, 2012 at 02:55:05PM +0700, Nguyễn Thái Ngọc Duy wrote:
> New test cases list commands that should work when memory is
> limited. All memory allocation functions (*) learn to reject any
> allocation larger than $GIT_ALLOC_LIMIT if set.
>
> (*) Not exactly all. Some places do not use x* functions, but
> malloc/calloc directly, notably diff-delta. These could path should
^code
> never be run on large blobs.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
-Peter
^ permalink raw reply
* Re: [PATCH] send-email: document the --smtp-debug option
From: Junio C Hamano @ 2012-02-27 20:19 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, greened
In-Reply-To: <1330359773-16759-1-git-send-email-zbyszek@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> The option was already shown in -h output, so it should be documented
> in the man page.
>
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> Suggested-by: David A. Greene <greened@obbligato.org>
> ---
> David Greene wrote:
>> I don't think --smtp-debug is documented in the man pages. Was that a
>> deliberate decision or an oversight?
Probably halfway in between "Meh"; anybody who is willing to look into the
issue will notice when he opens the script and reads it for the first time
anyway, so why bother.
But now "A patch already exists, and it does not seem to have any funny
letters that may screw up asciidoc, so why bother rejecting" ;-)
Will apply; thanks.
> Documentation/git-send-email.txt | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
> index 327233c..3241170 100644
> --- a/Documentation/git-send-email.txt
> +++ b/Documentation/git-send-email.txt
> @@ -198,6 +198,10 @@ must be used for each option.
> if a username is not specified (with '--smtp-user' or 'sendemail.smtpuser'),
> then authentication is not attempted.
>
> +--smtp-debug=0|1::
> + Enable (1) or disable (0) debug output. If enabled, SMTP
> + commands and replies will be printed. Useful to debug TLS
> + connection and authentication problems.
>
> Automating
> ~~~~~~~~~~
^ permalink raw reply
* Re: [PATCH] grep -P: add tests for matching ^ and $
From: Junio C Hamano @ 2012-02-27 20:21 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, gitster, michal.kiedrowicz
In-Reply-To: <1330361149-26741-1-git-send-email-zbyszek@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> From: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
>
> When a fix for matching ^ and $ with -P was commited to master in
> fba4f1 (grep -P: Fix matching ^ and $), the tests were missing the
> LIBPCRE prerequisite check and were dropped from the patch. Here are
> the tests guarded with LIBPCRE.
Thanks.
The real reason I separated it was because I didn't want to worry about
the test part when merging this down to older maintenance releases.
^ permalink raw reply
* Bug: post-receive-email sometimes misses commits
From: Michael Haggerty @ 2012-02-27 20:39 UTC (permalink / raw)
To: git discussion list; +Cc: Andy Parkins
In the following scenario, suppose that commit X is already in the
repository (referenced by master), and I push references A and B at the
same time:
-- master
/
o---X---*'--*'--* A
\
*---* B
The commits marked with "*" are new to the repository, and therefore the
emails generated by post-receive-email should summarize all five of
them. But in fact the commits marked with a prime (i.e., A^ and A^^)
are NOT included in any email.
The problem is that post-receive-email operates on one reference update
at a time, *after* the references have been updated. It generates the
"new" commits for A using the equivalent of
git rev-list ^master ^B A
and it generates the "new" commits for B using the equivalent of
git rev-list ^master ^B A
Neither of these ranges includes the commits common to branches A and B
and therefore those commits do not appear in any of the email notifications.
I first observed this problem in a slightly more complicated scenario
where a bugfix branch was merged to two release branches, then the two
release branches (but not the bugfix reference) were pushed:
o---o-------* release-A
/ /
o---o---o---*'--*'--*' bugfix
\ \
o---o-------* release-B
In this case only the merge commits were included in the commit emails;
the bugfixes commits themselves were never mentioned.
It seems like this problem can only be fixed by having
post-receive-email process all of the reference updates at once so that
the email generation for one reference update can take into account the
other references that were updated at the same time.
I don't plan to work on this problem; locally, we are using a Python
script that was derived from post-receive-email and enhanced in several
ways rather than using the Shell-script original. So when I have fixed
our Python script my own itch will have been scratched. If there is
interest in the Python email-notification script please let me know and
there is a chance I might clean it up enough to be released.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: Bug: post-receive-email sometimes misses commits
From: Michael Haggerty @ 2012-02-27 20:43 UTC (permalink / raw)
To: git discussion list; +Cc: Andy Parkins
In-Reply-To: <4F4BEA09.90600@alum.mit.edu>
On 02/27/2012 09:39 PM, Michael Haggerty wrote:
> [...] It generates the
> "new" commits for A using the equivalent of
>
> git rev-list ^master ^B A
>
> and it generates the "new" commits for B using the equivalent of
>
> git rev-list ^master ^B A
Sorry, that last line should of course be
git rev-list ^master ^A B
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-
From: Junio C Hamano @ 2012-02-27 20:48 UTC (permalink / raw)
To: René Scharfe
Cc: Thomas Rast, git, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <4F4BC3B3.7080000@lsrfire.ath.cx>
René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> The patch does not forbid adding "no-" to an option that already starts
> with "no-". This stricter rule would be easy to add, but since that is
> currently the only way to negate such options, it would break backwards
> compatibility and thus should be added in a separate patch, if at all.
>
> With the patch, the following guidelines are followed:
>
> - "no-" means no, for both developers and users.
> - The user doesn't have to to say "no-no-".
>
> The results feels simpler to me.
Sounds fair.
I agree that the backward compatibility of --no-no-foo is a potential
problem, if any of the actions controlled by "--no-foo" option defaults to
the behaviour when "--no-foo" is given. Among the existing 13 that you
listed, I do not think there is any that tempts any existing user to ask
for negation with "--no-no-foo" form, so I think we should be Ok.
^ permalink raw reply
* Re: [PATCH v2 1/2] CodingGuidelines: Add a note about spaces after redirection
From: Junio C Hamano @ 2012-02-27 20:55 UTC (permalink / raw)
To: Tim Henigan; +Cc: git
In-Reply-To: <1330125178-9194-1-git-send-email-tim.henigan@gmail.com>
Thanks; will apply both.
^ permalink raw reply
* Re: Announcing nntpgit
From: Michael Haggerty @ 2012-02-27 21:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Corbet, git
In-Reply-To: <7v62evykrq.fsf@alter.siamese.dyndns.org>
On 02/24/2012 10:38 PM, Junio C Hamano wrote:
> This is something I wanted to write (or see somebody write so that I can
> use it ;-)) even before I became the maintainer of this project, as I
> practically live inside GNUS, but never got around to go beyond the design
> phase.
>
> How do you handle message threading (References: and In-Reply-To:)?
>
> Would a commit on the "mainline" (a rough approximation of it would be
> "log --first-parent" starting from the tip) form the discussion starter
> article, and any side branch that fork from them would be a discussion
> thread starting at the commit?
>From contrib/hooks/post-receive-email I've locally derived a Python
version that sends one email message per commit. It generates a thread
of messages for each reference that was pushed, starting with a summary
of how the reference was changed, and one followup email for each new
commit added to that reference. In a threaded mail reader, this groups
all of the changes to a reference due to a particular push in a thread
that can easily be expanded, collapsed, or deleted as a group. I find
this a convenient workflow.
I believe that NNTP threading works much the same way as email
threading, so probably the same style could be used there. My script is
still not published, though.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: git-subtree Ready #2
From: Junio C Hamano @ 2012-02-27 21:06 UTC (permalink / raw)
To: David A. Greene; +Cc: Avery Pennarun, Jeff King, git
In-Reply-To: <87ty2ft0tm.fsf@smith.obbligato.org>
greened@obbligato.org (David A. Greene) writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>>> I'm happy to do either (rebase or filter-branch). Just let me know.
>>
>> I would understand Avery's "should we filter-branch/rebase, or is it OK
>> as-is?", but I do not understand what you mean by "either rebase or
>> filter-branch is fine".
>
> Sorry, got mixed up there. I'm not that familiar with filter-branch.
> Now I understand you do both. :)
>
> So have we decided to keep the history?
I think the discussion so far was:
- Peff suggested to keep the history with a true merge;
- I said the history before the final commit in Avery's tree did not look
so useful for future archaeology; and then
- Avery corrected me that there are contributions by other people and the
credits will be lost if we discarded the history;
and everybody (including me) now favors to have the history.
So the answer to your question is yes, but I do not think we heard opinion
from anybody regarding the question by Avery yet. I personally do not see
how it would help us if the old history is rewritten at this point.
^ permalink raw reply
* Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree
From: Johannes Sixt @ 2012-02-27 21:19 UTC (permalink / raw)
To: Jens Lehmann
Cc: Junio C Hamano, Git Mailing List, Antony Male, Phil Hord, msysGit
In-Reply-To: <4F4A8EF2.3020901@web.de>
Am 26.02.2012 20:58, schrieb Jens Lehmann:
> Am 26.02.2012 18:38, schrieb Johannes Sixt:
>> - a=$(cd "$gitdir" && pwd)
>> - b=$(cd "$path" && pwd)
>> + a=$(cd "$gitdir" && pwd -W)
>> + b=$(cd "$path" && pwd -W)
>> while [ "$b" ] && [ "${a%%/*}" = "${b%%/*}" ]
>> do
>> a=${a#*/} b=${b#*/};
>
> I don't understand why you need this. Does "pwd" sometimes return a
> path starting with "c:/" and sometimes "/c/" depending on what form
> you use when you cd into that directory?
It looks like this is the case. I was surprised as well. I hoped that
pwd -P would fix it, but it makes no difference. I should have tested
pwd -L as well, but I forgot.
> - gitdir=$(git rev-parse --git-dir)
> + gitdir=$(git rev-parse --git-dir | sed -e 's,^\([a-z]\):/,/\1/,')
I don't like pipelines of this kind because they fork yet another
process. But it looks like there are not that many alternatives...
-- Hannes
^ permalink raw reply
* Re: [PATCH] fsck: do not print dangling objects by default
From: Clemens Buchacher @ 2012-02-27 21:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vr4xg6pn2.fsf@alter.siamese.dyndns.org>
On Mon, Feb 27, 2012 at 11:29:53AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> Given that, isn't it not just sufficient but actually better to instead
> >> add a new --no-dangling option and keep the default unchanged?
> >
> > ... Of course, it is fsck, so I wonder how often clueless people are
> > really running it in the first place (i.e., it is not and should not be
> > part of most users' typical workflows). If it is simply the case that
> > they are being told to run "git fsck" by more expert users without
> > understanding what it does, then I could buy the argument that those
> > expert users could just as easily say "git fsck --no-dangling".
>
> Yes, that was certainly part of my pros-and-cons analysis. If you run
> "git fsck" without "--no-dangling" without reading the manual, you may
> get confused, but that is *not* the primary audience.
It is not my only concern that users might be confused. I believe the
command prints a lot of useless messages, which is by itself a UI
deficiency. But even worse, those numerous messages tend to hide an
actual problem in a long scrollback buffer. Sometimes my scrollback
buffer is not even large enough and I have to re-run fsck (which is not
exactly a fast command), just so I can grep out the dangling blobs.
> People who are curious can read the manual and figure it out, and the
> need for "fsck" is much rarer these days, compared to 2005 ;-)
In my opinion, the need for fsck is much more common these days. With
the alternates feature, it happens all the time that a repository breaks
if one is not extremely careful.
> In that context, only large downsides of potentially breaking and having
> to adjust existing scripts remains without much upsides, if we were to
> switch the default.
There is something wrong with weighting a UI improvement against
convenient use in scripts. If that were the issue, then we should add a
plumbing version for all commands, like we do for git status
--porcelain. Otherwise we can never change anything any more.
^ permalink raw reply
* Re: git-subtree Ready #2
From: Jeff King @ 2012-02-27 21:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David A. Greene, Avery Pennarun, git
In-Reply-To: <7vobsk56md.fsf@alter.siamese.dyndns.org>
On Mon, Feb 27, 2012 at 01:06:02PM -0800, Junio C Hamano wrote:
> >>> I'm happy to do either (rebase or filter-branch). Just let me know.
> >>
> >> I would understand Avery's "should we filter-branch/rebase, or is it OK
> >> as-is?", but I do not understand what you mean by "either rebase or
> >> filter-branch is fine".
> >
> > Sorry, got mixed up there. I'm not that familiar with filter-branch.
> > Now I understand you do both. :)
> >
> > So have we decided to keep the history?
>
> I think the discussion so far was:
>
> - Peff suggested to keep the history with a true merge;
>
> - I said the history before the final commit in Avery's tree did not look
> so useful for future archaeology; and then
>
> - Avery corrected me that there are contributions by other people and the
> credits will be lost if we discarded the history;
>
> and everybody (including me) now favors to have the history.
>
> So the answer to your question is yes, but I do not think we heard opinion
> from anybody regarding the question by Avery yet. I personally do not see
> how it would help us if the old history is rewritten at this point.
Yeah, I don't see much point in rewriting. If parts of the history suck,
then so be it. It's probably not that big to store. And while it's
sometimes easier to fix bad commit messages when they are recent and in
your memory (rather than trying to remember later what you meant to
say), I think it is already too late for that. Any archaeology you do
now to make good commit messages could probably just as easily be done
if and when somebody actually needs the commit message later (emphasis
on the "if" -- it's likely that nobody will care about most of the
commit messages later at all).
-Peff
^ permalink raw reply
* Re: git-subtree Ready #2
From: Jeff King @ 2012-02-27 21:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David A. Greene, Avery Pennarun, git
In-Reply-To: <20120227212157.GA19779@sigill.intra.peff.net>
On Mon, Feb 27, 2012 at 04:21:57PM -0500, Jeff King wrote:
> > So the answer to your question is yes, but I do not think we heard opinion
> > from anybody regarding the question by Avery yet. I personally do not see
> > how it would help us if the old history is rewritten at this point.
>
> Yeah, I don't see much point in rewriting. If parts of the history suck,
> then so be it. It's probably not that big to store. And while it's
> sometimes easier to fix bad commit messages when they are recent and in
> your memory (rather than trying to remember later what you meant to
> say), I think it is already too late for that. Any archaeology you do
> now to make good commit messages could probably just as easily be done
> if and when somebody actually needs the commit message later (emphasis
> on the "if" -- it's likely that nobody will care about most of the
> commit messages later at all).
Sorry, the "you" there is meant to be David. Forgot who I was responding
to for a minute.
-Peff
^ permalink raw reply
* Re: [PATCH] fsck: do not print dangling objects by default
From: Jeff King @ 2012-02-27 21:33 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: Junio C Hamano, git
In-Reply-To: <20120227211316.GA29081@ecki>
On Mon, Feb 27, 2012 at 10:13:16PM +0100, Clemens Buchacher wrote:
> > Yes, that was certainly part of my pros-and-cons analysis. If you run
> > "git fsck" without "--no-dangling" without reading the manual, you may
> > get confused, but that is *not* the primary audience.
>
> It is not my only concern that users might be confused. I believe the
> command prints a lot of useless messages, which is by itself a UI
> deficiency. But even worse, those numerous messages tend to hide an
> actual problem in a long scrollback buffer. Sometimes my scrollback
> buffer is not even large enough and I have to re-run fsck (which is not
> exactly a fast command), just so I can grep out the dangling blobs.
Yeah, but doesn't adding "--no-dangling" solve that issue?
> > People who are curious can read the manual and figure it out, and the
> > need for "fsck" is much rarer these days, compared to 2005 ;-)
>
> In my opinion, the need for fsck is much more common these days. With
> the alternates feature, it happens all the time that a repository breaks
> if one is not extremely careful.
Interesting. I can't remember the last time I ran fsck. But I am not the
only git user, and my assumptions are only based on my own experience,
not other data.
> > In that context, only large downsides of potentially breaking and having
> > to adjust existing scripts remains without much upsides, if we were to
> > switch the default.
>
> There is something wrong with weighting a UI improvement against
> convenient use in scripts. If that were the issue, then we should add a
> plumbing version for all commands, like we do for git status
> --porcelain. Otherwise we can never change anything any more.
I don't think it's convenience as much as scripts which try to detect
dangling by using "git fsck" will now be broken with a really bad
failure mode (i.e., they used to run "git fsck" and parse the dangling
lines on stderr, but now they will erroneously think that nothing is
dangling!).
If we really care about that, then the right answer is our usual:
1. Introduce --dangling/--no-dangling, leaving the default.
2. Wait a while until the features are well-established.
3. Scripts adapt to use --dangling if they want it.
4. Wait a while for scripts to be updated.
5. Flip the default to --no-dangling.
I wrote but didn't send in my last email a long analysis of why it is
wrong to combine fsck's default mode of operation (which is really about
"is this repository corrupted or broken") with checking for dangling
objects (which is something you do when you are looking for lost work).
But I don't think that is even really up for debate. If we were
designing fsck from scratch, I think everybody in the thread has the
feeling that reporting dangling by default is wrong. And we should
definitely add "--no-dangling" to address that.
But flipping the default is a different story, and that has to weigh the
potential breakage above versus having a good UI. I'm on the fence about
whether the above is a big enough problem to care about (mostly because
I'm not sure anybody has actually written such a script).
-Peff
^ permalink raw reply
* Re: [PATCH] fsck: do not print dangling objects by default
From: Junio C Hamano @ 2012-02-27 21:34 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: Jeff King, git
In-Reply-To: <20120227211316.GA29081@ecki>
Clemens Buchacher <drizzd@aon.at> writes:
> There is something wrong with weighting a UI improvement against
> convenient use in scripts. If that were the issue, then ...
But that is not the issue. Nobody said anything about convenience for
script writers.
I am talking about regressions to already written long time ago and used
by many people _without_ them having to worry about how it internally
works. By switching the default, you are suddenly making them worry about
it again.
^ permalink raw reply
* Re: [msysGit] Re: [PATCH 0/2] submodules: Use relative paths to gitdir and work tree
From: Johannes Schindelin @ 2012-02-27 21:41 UTC (permalink / raw)
To: Johannes Sixt
Cc: Jens Lehmann, Junio C Hamano, Git Mailing List, Antony Male,
Phil Hord, msysGit
In-Reply-To: <4F4BF357.8020407@kdbg.org>
Hi,
On Mon, 27 Feb 2012, Johannes Sixt wrote:
> Am 26.02.2012 20:58, schrieb Jens Lehmann:
>
> > - gitdir=$(git rev-parse --git-dir)
> > + gitdir=$(git rev-parse --git-dir | sed -e 's,^\([a-z]\):/,/\1/,')
>
> I don't like pipelines of this kind because they fork yet another
> process. But it looks like there are not that many alternatives...
Not many, but some rather straight-forward ones. E.g.
# quoted to preserve tabs & newslines (Windows users are creative)
gitdir="$(git rev-parse --git-dir)"
case "$gitdir" in
[A-Za-z]:*)
gitdir="/${gitdir%%:*}${gitdir#?:}"
;;
esac
Note: untested.
Note2: I did not add error handling, either.
Note3: yes, ${...} is available in bash and it does not fork. Neither does
case ... esac
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 02/11] Factor out and export large blob writing code to arbitrary file handle
From: Junio C Hamano @ 2012-02-27 21:50 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <7v4nucb2xl.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> So I think the external declaration and the definition should move to a
> more generic place, namely streaming.[ch]. It does not belong to entry.c
> anymore.
>
> Thanks for working on this.
In other words, I think the result should look more like this.
The original logic in entry.c is that the caller should try to get a
filter and call streaming_write_entry(), but either of them is allowed to
return a failure when the blob is not suitable for the streaming codepath
to tell the caller to try their traditional codepath.
We might want to add another helper function for callers to use to decide
if they should use the streaming interface, or the traditional one, before
actually making a call to streaming_write_entry(). With the original (and
current) API, they have to retry even when the streaming codepath truly
failed (e.g. no such blob object), in which case it is very likely that
the traditional codepath in the caller will fail the same way. Retrying is
a wasted effort in such a case.
-- >8 --
Subject: [PATCH] streaming: make streaming-write-entry to be more reusable
The static function in entry.c takes a cache entry and streams its blob
contents to a file in the working tree. Refactor the logic to a new API
function stream_blob_to_fd() that takes an object name and an open file
descriptor, so that it can be reused by other callers.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
entry.c | 53 +++++------------------------------------------------
streaming.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
streaming.h | 2 ++
3 files changed, 62 insertions(+), 48 deletions(-)
diff --git a/entry.c b/entry.c
index 852fea1..17a6bcc 100644
--- a/entry.c
+++ b/entry.c
@@ -120,58 +120,15 @@ static int streaming_write_entry(struct cache_entry *ce, char *path,
const struct checkout *state, int to_tempfile,
int *fstat_done, struct stat *statbuf)
{
- struct git_istream *st;
- enum object_type type;
- unsigned long sz;
int result = -1;
- ssize_t kept = 0;
- int fd = -1;
-
- st = open_istream(ce->sha1, &type, &sz, filter);
- if (!st)
- return -1;
- if (type != OBJ_BLOB)
- goto close_and_exit;
+ int fd;
fd = open_output_fd(path, ce, to_tempfile);
- if (fd < 0)
- goto close_and_exit;
-
- for (;;) {
- char buf[1024 * 16];
- ssize_t wrote, holeto;
- ssize_t readlen = read_istream(st, buf, sizeof(buf));
-
- if (!readlen)
- break;
- if (sizeof(buf) == readlen) {
- for (holeto = 0; holeto < readlen; holeto++)
- if (buf[holeto])
- break;
- if (readlen == holeto) {
- kept += holeto;
- continue;
- }
- }
-
- if (kept && lseek(fd, kept, SEEK_CUR) == (off_t) -1)
- goto close_and_exit;
- else
- kept = 0;
- wrote = write_in_full(fd, buf, readlen);
-
- if (wrote != readlen)
- goto close_and_exit;
- }
- if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||
- write(fd, "", 1) != 1))
- goto close_and_exit;
- *fstat_done = fstat_output(fd, state, statbuf);
-
-close_and_exit:
- close_istream(st);
- if (0 <= fd)
+ if (0 <= fd) {
+ result = stream_blob_to_fd(fd, ce->sha1, filter, 1);
+ *fstat_done = fstat_output(fd, state, statbuf);
result = close(fd);
+ }
if (result && 0 <= fd)
unlink(path);
return result;
diff --git a/streaming.c b/streaming.c
index 71072e1..7e7ee2b 100644
--- a/streaming.c
+++ b/streaming.c
@@ -489,3 +489,58 @@ static open_method_decl(incore)
return st->u.incore.buf ? 0 : -1;
}
+
+
+/****************************************************************
+ * Users of streaming interface
+ ****************************************************************/
+
+int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *filter,
+ int can_seek)
+{
+ struct git_istream *st;
+ enum object_type type;
+ unsigned long sz;
+ ssize_t kept = 0;
+ int result = -1;
+
+ st = open_istream(sha1, &type, &sz, filter);
+ if (!st)
+ return result;
+ if (type != OBJ_BLOB)
+ goto close_and_exit;
+ for (;;) {
+ char buf[1024 * 16];
+ ssize_t wrote, holeto;
+ ssize_t readlen = read_istream(st, buf, sizeof(buf));
+
+ if (!readlen)
+ break;
+ if (can_seek && sizeof(buf) == readlen) {
+ for (holeto = 0; holeto < readlen; holeto++)
+ if (buf[holeto])
+ break;
+ if (readlen == holeto) {
+ kept += holeto;
+ continue;
+ }
+ }
+
+ if (kept && lseek(fd, kept, SEEK_CUR) == (off_t) -1)
+ goto close_and_exit;
+ else
+ kept = 0;
+ wrote = write_in_full(fd, buf, readlen);
+
+ if (wrote != readlen)
+ goto close_and_exit;
+ }
+ if (kept && (lseek(fd, kept - 1, SEEK_CUR) == (off_t) -1 ||
+ write(fd, "", 1) != 1))
+ goto close_and_exit;
+ result = 0;
+
+ close_and_exit:
+ close_istream(st);
+ return result;
+}
diff --git a/streaming.h b/streaming.h
index 589e857..3e82770 100644
--- a/streaming.h
+++ b/streaming.h
@@ -12,4 +12,6 @@ extern struct git_istream *open_istream(const unsigned char *, enum object_type
extern int close_istream(struct git_istream *);
extern ssize_t read_istream(struct git_istream *, char *, size_t);
+extern int stream_blob_to_fd(int fd, const unsigned char *, struct stream_filter *, int can_seek);
+
#endif /* STREAMING_H */
--
1.7.9.2.312.g1abc3
^ permalink raw reply related
* Re: [RFC/PATCH] Make git-{pull,rebase} no-tracking message friendlier
From: Matthieu Moy @ 2012-02-27 22:06 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git
In-Reply-To: <1330013115-26355-1-git-send-email-cmn@elego.de>
Carlos Martín Nieto <cmn@elego.de> writes:
> - echo "You asked me to $cmd without telling me which branch you
> -want to $op_type $op_prep, and 'branch.${branch_name#refs/heads/}.merge' in
> -your configuration file does not tell me, either. Please
> -specify which branch you want to use on the command line and
> + echo "You asked me to $cmd without telling me which branch you want to
> +$op_type $op_prep, and there is no tracking information for the current branch.
> +Please specify which branch you want to use on the command line and
> try again (e.g. '$example').
At this point, it may be better to actually give the full command
instead of just this "(e.g. '$example')", i.e. stg like
git $op_type <remote> $example
I also saw users confused by the message (indeed without reading it,
but ...). Giving them something as close as possible to
cut-and-paste-able command should help.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v6 02/11] Add git-column and column mode parsing
From: Ramsay Jones @ 2012-02-27 20:09 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Junio C Hamano
In-Reply-To: <1330170078-29353-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy wrote:
[...]
> +static int parse_option(const char *arg, int len,
> + unsigned int *mode, int stdout_is_tty)
> +{
> + struct colopt opts[] = {
> + { ENABLE, "always", 1 },
> + { ENABLE, "never", 0 },
> + { ENABLE, "auto", -1 },
> + };
Hmm, I don't recognise this table from last time ...
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(opts); i++) {
> + int set = 1, arg_len = len, name_len;
set is initialised here (mainly to silence the compiler) in each
loop, but then ...
> + const char *arg_str = arg;
> +
> + if (opts[i].type == OPTION) {
> + if (arg_len > 2 && !strncmp(arg_str, "no", 2)) {
> + arg_str += 2;
> + arg_len -= 2;
> + set = 0;
> + } else {
> + set = 1;
... this else clause is no longer required.
> + }
> + }
> +
> + name_len = strlen(opts[i].name);
> + if (arg_len != name_len ||
> + strncmp(arg_str, opts[i].name, name_len))
> + continue;
> +
> + switch (opts[i].type) {
> + case ENABLE:
> + return set_enable_bit(mode, opts[i].value,
> + stdout_is_tty);
given the above table, can the following case limbs ever be reached?
(the "no" prefix is only applied to the OPTION type, so most of the
above code seems to be useless now ...)
> + case MODE:
> + return set_mode(mode, opts[i].value);
> + case OPTION:
> + return set_option(mode, opts[i].value, set);
> + default:
> + die("BUG: Unknown option type %d", opts[i].type);
> + }
> + }
> +
> + return error("unsupported style '%s'", arg);
> +}
> +
[...]
Note, I only skimmed the patch text, I haven't applied it or tested it ...
ATB,
Ramsay Jones
^ permalink raw reply
* [PATCH 1/2] builtin/symbolic-ref.c: add option to fall back to normal ref
From: Jan Krüger @ 2012-02-27 22:08 UTC (permalink / raw)
To: git
Frequently, people want to determine the current value of HEAD in
scripts. However, there is no tool that can always output it, since "git
symbolic-ref" will fail if HEAD isn't currently a symref, and other
tools (e.g. "git rev-parse --symbolic-full-name") will also fail in
one of HEAD's possible modes.
To resolve this situation, add the new -f option to symbolic-ref that
falls back to outputting the value of HEAD as a normal ref if necessary.
Signed-off-by: Jan Krüger <jk@jk.gs>
---
Documentation/git-symbolic-ref.txt | 7 ++++++-
builtin/symbolic-ref.c | 16 ++++++++++++++--
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-symbolic-ref.txt b/Documentation/git-symbolic-ref.txt
index a45d4c4..a05819b 100644
--- a/Documentation/git-symbolic-ref.txt
+++ b/Documentation/git-symbolic-ref.txt
@@ -8,7 +8,7 @@ git-symbolic-ref - Read and modify symbolic refs
SYNOPSIS
--------
[verse]
-'git symbolic-ref' [-q] [-m <reason>] <name> [<ref>]
+'git symbolic-ref' [-q] [-f] [-m <reason>] <name> [<ref>]
DESCRIPTION
-----------
@@ -33,6 +33,11 @@ OPTIONS
symbolic ref but a detached HEAD; instead exit with
non-zero status silently.
+-f::
+ When showing the current value of <name>, do not fail if it is
+ not a symbolic ref; instead output the SHA1 value referenced by
+ <name>.
+
-m::
Update the reflog for <name> with <reason>. This is valid only
when creating or updating a symbolic ref.
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 2ef5962..2e0a86f 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -8,6 +8,8 @@ static const char * const git_symbolic_ref_usage[] = {
NULL
};
+static int fallback_regular_ref;
+
static void check_symref(const char *HEAD, int quiet)
{
unsigned char sha1[20];
@@ -17,10 +19,18 @@ static void check_symref(const char *HEAD, int quiet)
if (!refs_heads_master)
die("No such ref: %s", HEAD);
else if (!(flag & REF_ISSYMREF)) {
- if (!quiet)
+ if (fallback_regular_ref) {
+ char sha1[20];
+ if (!get_sha1(HEAD, sha1))
+ puts(sha1_to_hex(sha1));
+ else
+ die("failed to resolve ref %s", HEAD);
+ return;
+ } else if (!quiet) {
die("ref %s is not a symbolic ref", HEAD);
- else
+ } else {
exit(1);
+ }
}
puts(refs_heads_master);
}
@@ -32,6 +42,8 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
struct option options[] = {
OPT__QUIET(&quiet,
"suppress error message for non-symbolic (detached) refs"),
+ OPT_BOOLEAN('f', NULL, &fallback_regular_ref,
+ "fall back to showing as a regular ref"),
OPT_STRING('m', NULL, &msg, "reason", "reason of the update"),
OPT_END(),
};
--
1.7.9.2.302.g3724c.dirty
From 1fffb746a65aac88d3af9bae785b1cfa58cbf31c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jan=20Kr=C3=BCger?= <jk@jk.gs>
Date: Mon, 27 Feb 2012 22:40:13 +0100
Subject: [PATCH 2/2] builtin/symbolic-ref.c: add option to output shortened
ref
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
In scripts meant to generate user-consumable output, it can be helpful
to resolve a symbolic ref and output the result in a shortened form,
such as for use in shell prompts. Add a new -s option to allow this.
Signed-off-by: Jan Krüger <jk@jk.gs>
---
Documentation/git-symbolic-ref.txt | 6 +++++-
builtin/symbolic-ref.c | 5 +++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-symbolic-ref.txt b/Documentation/git-symbolic-ref.txt
index a05819b..7f108ce 100644
--- a/Documentation/git-symbolic-ref.txt
+++ b/Documentation/git-symbolic-ref.txt
@@ -8,7 +8,7 @@ git-symbolic-ref - Read and modify symbolic refs
SYNOPSIS
--------
[verse]
-'git symbolic-ref' [-q] [-f] [-m <reason>] <name> [<ref>]
+'git symbolic-ref' [-q] [-f] [-s] [-m <reason>] <name> [<ref>]
DESCRIPTION
-----------
@@ -38,6 +38,10 @@ OPTIONS
not a symbolic ref; instead output the SHA1 value referenced by
<name>.
+-s::
+ When showing the value of <name> as a symbolic ref, try to shorten the
+ value, e.g. from `refs/heads/master` to `master`.
+
-m::
Update the reflog for <name> with <reason>. This is valid only
when creating or updating a symbolic ref.
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 2e0a86f..df8da11 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -9,6 +9,7 @@ static const char * const git_symbolic_ref_usage[] = {
};
static int fallback_regular_ref;
+static int shorten;
static void check_symref(const char *HEAD, int quiet)
{
@@ -32,6 +33,9 @@ static void check_symref(const char *HEAD, int quiet)
exit(1);
}
}
+ if (shorten)
+ refs_heads_master = shorten_unambiguous_ref(
+ refs_heads_master, 0);
puts(refs_heads_master);
}
@@ -44,6 +48,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
"suppress error message for non-symbolic (detached) refs"),
OPT_BOOLEAN('f', NULL, &fallback_regular_ref,
"fall back to showing as a regular ref"),
+ OPT_BOOLEAN('s', NULL, &shorten, "shorten ref output"),
OPT_STRING('m', NULL, &msg, "reason", "reason of the update"),
OPT_END(),
};
--
1.7.9.2.302.g3724c.dirty
^ permalink raw reply related
* [PATCH 2/2] builtin/symbolic-ref.c: add option to output shortened ref
From: Jan Krüger @ 2012-02-27 22:10 UTC (permalink / raw)
To: git
In scripts meant to generate user-consumable output, it can be helpful
to resolve a symbolic ref and output the result in a shortened form,
such as for use in shell prompts. Add a new -s option to allow this.
Signed-off-by: Jan Krüger <jk@jk.gs>
---
Documentation/git-symbolic-ref.txt | 6 +++++-
builtin/symbolic-ref.c | 5 +++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-symbolic-ref.txt b/Documentation/git-symbolic-ref.txt
index a05819b..7f108ce 100644
--- a/Documentation/git-symbolic-ref.txt
+++ b/Documentation/git-symbolic-ref.txt
@@ -8,7 +8,7 @@ git-symbolic-ref - Read and modify symbolic refs
SYNOPSIS
--------
[verse]
-'git symbolic-ref' [-q] [-f] [-m <reason>] <name> [<ref>]
+'git symbolic-ref' [-q] [-f] [-s] [-m <reason>] <name> [<ref>]
DESCRIPTION
-----------
@@ -38,6 +38,10 @@ OPTIONS
not a symbolic ref; instead output the SHA1 value referenced by
<name>.
+-s::
+ When showing the value of <name> as a symbolic ref, try to shorten the
+ value, e.g. from `refs/heads/master` to `master`.
+
-m::
Update the reflog for <name> with <reason>. This is valid only
when creating or updating a symbolic ref.
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 2e0a86f..df8da11 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -9,6 +9,7 @@ static const char * const git_symbolic_ref_usage[] = {
};
static int fallback_regular_ref;
+static int shorten;
static void check_symref(const char *HEAD, int quiet)
{
@@ -32,6 +33,9 @@ static void check_symref(const char *HEAD, int quiet)
exit(1);
}
}
+ if (shorten)
+ refs_heads_master = shorten_unambiguous_ref(
+ refs_heads_master, 0);
puts(refs_heads_master);
}
@@ -44,6 +48,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
"suppress error message for non-symbolic (detached) refs"),
OPT_BOOLEAN('f', NULL, &fallback_regular_ref,
"fall back to showing as a regular ref"),
+ OPT_BOOLEAN('s', NULL, &shorten, "shorten ref output"),
OPT_STRING('m', NULL, &msg, "reason", "reason of the update"),
OPT_END(),
};
--
1.7.9.2.302.g3724c.dirty
^ permalink raw reply related
* Re: [PATCH 1/2] builtin/symbolic-ref.c: add option to fall back to normal ref
From: Junio C Hamano @ 2012-02-27 22:21 UTC (permalink / raw)
To: Jan Krüger; +Cc: git
In-Reply-To: <1330380536-9647-1-git-send-email-jk@jk.gs>
Jan Krüger <jk@jk.gs> writes:
> Frequently, people want to determine the current value of HEAD in
> scripts. However, there is no tool that can always output it, since "git
> symbolic-ref" will fail if HEAD isn't currently a symref, and other
> tools (e.g. "git rev-parse --symbolic-full-name") will also fail in
> one of HEAD's possible modes.
What is "the current value of HEAD"?
The symbolic-ref command is there for people who _care_ about the
distinction between a HEAD that points at a branch and a HEAD that points
directly at a commit. There is no room for the command to "fall back"
anywhere, as that will only introduce an unnecessary ambiguity to the
command whose sole purpose is to be able to tell them apart.
If the caller does not need to know if the HEAD is detached or not, and
wants to know what commit it points at, why is it insufficient to just use
rev-parse, e.g. "git rev-parse --verify HEAD"?
^ permalink raw reply
* Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP
From: René Scharfe @ 2012-02-27 22:26 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <20120227182504.GA1600@sigill.intra.peff.net>
Am 27.02.2012 19:25, schrieb Jeff King:
> On Sat, Feb 25, 2012 at 08:15:56PM +0100, René Scharfe wrote:
>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index e4ea900..b151467 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -671,7 +671,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>> struct string_list path_list = STRING_LIST_INIT_NODUP;
>> int i;
>> int dummy;
>> - int use_index = 1;
>> + int no_index = 0;
>> enum {
>> pattern_type_unspecified = 0,
>> pattern_type_bre,
>> @@ -684,9 +684,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>> struct option options[] = {
>> OPT_BOOLEAN(0, "cached",&cached,
>> "search in index instead of in the work tree"),
>> - { OPTION_BOOLEAN, 0, "index",&use_index, NULL,
>> - "finds in contents not managed by git",
>> - PARSE_OPT_NOARG | PARSE_OPT_NEGHELP },
>> + OPT_BOOL(0, "no-index",&no_index,
>> + "finds in contents not managed by git"),
>> OPT_BOOLEAN(0, "untracked",&untracked,
>> "search in both tracked and untracked files"),
>> OPT_SET_INT(0, "exclude-standard",&opt_exclude,
>> @@ -851,7 +850,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>> break; /* nothing */
>> }
>>
>> - if (use_index&& !startup_info->have_repository)
>> + if (!no_index&& !startup_info->have_repository)
[Unrelated: The whitespace in the two lines above and before ampersands
in general was damaged by Thunderbird. First time I noticed.]
> Hmm. We usually try to avoid these sorts of double negations in the
> code, as they can often make the logic hard to read. In this case, it is
> not _so_ bad, because out of the 4 uses of use_index/no_index, only one
> is "!no_index", and it is in a relatively simple conditional.
The variable could be named "unmanaged", "external" or similar instead
of "no_index". The latter just matches the option name and thus was the
obvious first choice to me.
> But I do feel like the original was slightly easier to read, and that
> getting rid of NEGHELP is restricting how the developer can express the
> options.
>
> I think your original motivation was that NEGHELP lead to confusion
> where the name of the option does not match its description. Would it be
> better to simply be explicit that an option is a reversed boolean (i.e.,
> what the user specifies on the command line and what is in the code are
> naturally opposites). Like:
>
> OPT_REVERSE_BOOL(0, "no-index",&use_index,
> "finds in contents not managed by git"),
It's better than NEGHELP, but I find your use of two negations (REVERSE
and "no-") confusing. We don't need to invent new OPT_ types for this,
by the way, we can just do this:
OPT_NEGBIT(0, "no-index", &use_index,
"finds in contents not managed by git", 1),
It certainly shortens the patch.
> Using NEGHELP, the "reverse" is between the option name and the
> description, which is very subtle. Here it is between the option name
> and the variable, which is hopefully a little more explicit (especially
> with the big REVERSE in the macro name).
We have precedence for OPT_NEGBIT in grep, although the double negations
for -h and --full-name are required because both turn off bits that
other options turn on, while for --no-index it wouldn't be strictly
needed, as there is no option that overrules it except --index.
I don't care too much either way, though. The changes from patch 2 (the
no no-no one) are not restricted to OPT_BOOL.
> I dunno. Given that there are only two uses of NEGHELP, and that they
> don't come out too badly, I don't care _too_ much. But I have seen some
> really tortured logic with double-negations like this, and I'm concerned
> that a few months down the road somebody is going to want NEGHELP (or
> something similar) in a case where it actually does really impact
> readability.
I'm curious to see a case that can be solved better using NEGHELP, but
we can always add it back if we find such a beast. I'd much rather see
it go until then because of it's non-obvious semantics.
René
^ permalink raw reply
* Re: [PATCH] fsck: do not print dangling objects by default
From: Clemens Buchacher @ 2012-02-27 22:18 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20120227213304.GB19779@sigill.intra.peff.net>
On Mon, Feb 27, 2012 at 04:33:04PM -0500, Jeff King wrote:
> On Mon, Feb 27, 2012 at 10:13:16PM +0100, Clemens Buchacher wrote:
>
> > > Yes, that was certainly part of my pros-and-cons analysis. If you run
> > > "git fsck" without "--no-dangling" without reading the manual, you may
> > > get confused, but that is *not* the primary audience.
> >
> > It is not my only concern that users might be confused. I believe the
> > command prints a lot of useless messages, which is by itself a UI
> > deficiency. But even worse, those numerous messages tend to hide an
> > actual problem in a long scrollback buffer. Sometimes my scrollback
> > buffer is not even large enough and I have to re-run fsck (which is not
> > exactly a fast command), just so I can grep out the dangling blobs.
>
> Yeah, but doesn't adding "--no-dangling" solve that issue?
I can just as well use grep -v ^dangling.
^ 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