* [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
* [PATCH v2 1/2] CodingGuidelines: Add a note about spaces after redirection
From: Tim Henigan @ 2012-02-24 23:12 UTC (permalink / raw)
To: git, gitster; +Cc: tim.henigan
During code review of some patches, it was noted that redirection operators
should have space before, but no space after them.
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
Updated to include double-quotes around redirection target and also
document why they are needed.
Documentation/CodingGuidelines | 10 ++++++++++
1 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 4830086..5a190b9 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -35,6 +35,16 @@ For shell scripts specifically (not exhaustive):
- Case arms are indented at the same depth as case and esac lines.
+ - Redirection operators should be written with space before, but
+ no space after them. For example:
+ 'echo test >"$file"' is preferred over
+ 'echo test > "$file"'
+
+ Note that even though it is not required by POSIX to double-
+ quote the redirection target in a variable (as shown above),
+ our code does so because some versions of bash issue a warning
+ without them.
+
- We prefer $( ... ) for command substitution; unlike ``, it
properly nests. It should have been the way Bourne spelled
it from day one, but unfortunately isn't.
--
1.7.9.1
^ permalink raw reply related
* Re: [PATCH] branch: don't assume the merge filter ref exists
From: Jeff King @ 2012-02-27 19:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, Bernhard Reutner-Fischer, git
In-Reply-To: <7vk4386pgi.fsf@alter.siamese.dyndns.org>
On Mon, Feb 27, 2012 at 11:33:49AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > You would also get NULL if the object exists but is not a commit. Maybe:
> >
> > die("object '%s' does not point to a commit", ...)
> >
> > would be better? It covers the wrong-type case, and is still technically
> > true when the object does not exist.
>
> For this particular message I like the above a lot better. The output
> from "git grep -e 'invalid object' -e 'bad object'" seems to show that
> the use of both are fairly evenly distributed.
It looks like "bad object" generally comes from parse_object failing,
which makes sense. It either means object corruption or you fed a full
40-char sha1 that didn't exist (which, if you are being that specific,
probably is an indication of broken-ness in your repository).
It looks like "invalid object" comes from failing to access the subject
of an annotated tag or an entry in a tree, both of which would meet the
same criteria (corruption or a missing 40-char sha1).
I don't think bad versus invalid in existing cases is a big deal, as
they are both used consistently. But in this case, I think either would
be wrong, since it is equally likely that the user gave an existing, OK
object of the wrong type.
-Peff
^ permalink raw reply
* Re: [PATCH] branch: don't assume the merge filter ref exists
From: Junio C Hamano @ 2012-02-27 19:33 UTC (permalink / raw)
To: Jeff King; +Cc: Carlos Martín Nieto, Bernhard Reutner-Fischer, git
In-Reply-To: <20120227193044.GD1600@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> You would also get NULL if the object exists but is not a commit. Maybe:
>
> die("object '%s' does not point to a commit", ...)
>
> would be better? It covers the wrong-type case, and is still technically
> true when the object does not exist.
For this particular message I like the above a lot better. The output
from "git grep -e 'invalid object' -e 'bad object'" seems to show that
the use of both are fairly evenly distributed.
^ permalink raw reply
* Re: [PATCH] branch: don't assume the merge filter ref exists
From: Jeff King @ 2012-02-27 19:30 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Bernhard Reutner-Fischer, git, Junio C Hamano
In-Reply-To: <1330355513-22351-1-git-send-email-cmn@elego.de>
On Mon, Feb 27, 2012 at 04:11:53PM +0100, Carlos Martín Nieto wrote:
> print_ref_list looks up the merge_filter_ref and assumes that a valid
> pointer is returned. When the object doesn't exist, it tries to
> dereference a NULL pointer. This can be the case when git branch
> --merged is given an argument that isn't a valid commit name.
>
> Check whether the lookup returns a NULL pointer and die with an error
> if it does. Add a test, while we're at it.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>
> It certainly looks like --merged was only ever supposed to be used
> with branch names, as it assumed that get_sha1() would catch the
> errors.
>
> I'm not sure if "bad object" or "invalid object" fits better. "bad
> object" might have a stronger implication that it exists but is
> corrupt.
You would also get NULL if the object exists but is not a commit. Maybe:
die("object '%s' does not point to a commit", ...)
would be better? It covers the wrong-type case, and is still technically
true when the object does not exist.
-Peff
^ permalink raw reply
* Re: [PATCH] fsck: do not print dangling objects by default
From: Junio C Hamano @ 2012-02-27 19:29 UTC (permalink / raw)
To: Jeff King; +Cc: Clemens Buchacher, git
In-Reply-To: <20120227191846.GB1600@sigill.intra.peff.net>
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. 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 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.
^ permalink raw reply
* Re: git-cherries
From: Jeff King @ 2012-02-27 19:27 UTC (permalink / raw)
To: Thien-Thi Nguyen; +Cc: git
In-Reply-To: <874nucee98.fsf@gnuvola.org>
On Mon, Feb 27, 2012 at 11:56:19AM +0100, Thien-Thi Nguyen wrote:
> For my personal use, i wrote git-cherries, attached.
> It commits each hunk of every modified file separately
> (creating cherries to cherry-pick later, you see).
>
> I am writing to ask if this is already in Git somewhere,
> and if not, for tips on how to make it faster / more elegant.
So if I understand correctly, this just creates a series of commits, one
per hunk, of what's in your working tree. And the commit messages won't
be useful, so this is really about recording the work somewhere so that
you can pick it out later using "git cherry-pick --no-commit", make a
real commit from some subset of the cherries, and then throw away the
cherries?
I think you could do this more simply by putting everything in a single
throw-away commit, then using "git checkout -p $throwaway" to pick the
individual cherries from the single commit. You don't grab the commit
message from $throwaway as you might with cherry-pick, but by definition
it's not a very good commit message anyway.
-Peff
^ permalink raw reply
* Re: [PATCH] fsck: do not print dangling objects by default
From: Jeff King @ 2012-02-27 19:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Clemens Buchacher, git
In-Reply-To: <7vhayddxgp.fsf@alter.siamese.dyndns.org>
On Sun, Feb 26, 2012 at 02:46:46PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > I think that both the ultimate goal explained above, and the direction in
> > which the documentation updates tries to move us, are good. I only gave a
> > cursory look at the code changes, but what they implement seems to match
> > the intention.
> >
> > Of course I may be missing something, so objections from others to argue
> > why we shouldn't do this is very much welcomed to stop me and Clemens ;-).
>
> Let's start with the obvious.
>
> It is much easier for a user to use a new option on the command line when
> he wants to use an improved behaviour when he runs the command manually.
> Having to update scripts that run the command to act on its output, on the
> other hand, is much more painful to the users.
>
> And the intended audience for this change clearly is interactive users
> that follow the user-manual to try things out.
>
> Given that, isn't it not just sufficient but actually better to instead
> add a new --no-dangling option and keep the default unchanged?
But if your intended audience is users who are confused by the dangling
warnings, explaining to them to use --no-dangling is not really
improving the situation. 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".
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] Trivial cleanups in the post-receive-email script
From: Junio C Hamano @ 2012-02-27 19:01 UTC (permalink / raw)
To: mhagger; +Cc: git, Kevin P. Fleming, Andy Parkins
In-Reply-To: <1330367650-23091-1-git-send-email-mhagger@alum.mit.edu>
Shows that nobody uses these sample script.
Will apply. I wish all the patches were this easy ;-).
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP
From: Junio C Hamano @ 2012-02-27 18:58 UTC (permalink / raw)
To: Jeff King
Cc: René Scharfe, git, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <20120227182504.GA1600@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... 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"),
You said it much better than my attempt ;-).
> 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).
>
> 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.
Yeah, I share a similar minor and iffy feeling about the result.
^ permalink raw reply
* Re: [PATCH 2/2] CodingGuidelines: Add note forbidding use of 'which' in shell scripts
From: Junio C Hamano @ 2012-02-27 18:56 UTC (permalink / raw)
To: Tim Henigan; +Cc: git
In-Reply-To: <1330117921-8257-2-git-send-email-tim.henigan@gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index a4ffe7c..3505a4b 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -44,6 +44,10 @@ For shell scripts specifically (not exhaustive):
> properly nests. It should have been the way Bourne spelled
> it from day one, but unfortunately isn't.
>
> + - The use of 'which' is not allowed. The output of 'which' is not
> + machine parseable and its exit code is not reliable across
> + platforms.
It is more helpful to say "If you want to do Z, use X, not Y because Y is
broken for such and such reasons", rather than "Never use Y because Y is
broken ...".
In this case, Z is "find out if a command is available on user's $PATH",
and X is "type", I think.
^ permalink raw reply
* Re: [PATCH 1/2] CodingGuidelines: Add a note about spaces after redirection
From: Junio C Hamano @ 2012-02-27 18:55 UTC (permalink / raw)
To: Tim Henigan; +Cc: git, gitster
In-Reply-To: <1330117921-8257-1-git-send-email-tim.henigan@gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> + - Redirection operators should be written with space before, but
> + no space after them. For example:
> + 'echo test >$file' is preferred over
> + 'echo test > $file'
> +
If you are using a $file placeholder, then we would need to show readers
that they need to be enclosed in dq to prevent some versions of bash from
giving us a false warning, and explicitly say why.
$ bash
bash$ file='/var/tmp/f i l e'
bash$ >$file
bash: $file: ambiguous redirect
bash$ >"$file"
bash$ ls -1 "$file"
/var/tmp/f i l e
Adding something like this after your two line examples, after updating
them to use "$file" instead, should be sufficient:
Note that even though it is not required by POSIX to double quote
the redirection target in a variable like the above example, our
code does so because some versions of bash issue an warning unless
we do.
^ permalink raw reply
* Re: [PATCH] git-p4: missing she-bang line in t9804 confuses prove
From: Junio C Hamano @ 2012-02-27 18:47 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, luke
In-Reply-To: <1330364414-29332-1-git-send-email-zbyszek@in.waw.pl>
Thanks.
^ permalink raw reply
* Re: [PATCH 00/11] Large blob fixes
From: Junio C Hamano @ 2012-02-27 18:43 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330329315-11407-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> These patches make sure we avoid keeping whole blob in memory, at
> least in common cases. Blob-only streaming code paths are opened to
> accomplish that.
Some in the series seem to be unrelated to the above, namely, the
index-pack ones.
^ permalink raw reply
* Re: sha-1 check in rev-list --verify-objects redundant?
From: Junio C Hamano @ 2012-02-27 18:41 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Git Mailing List
In-Reply-To: <20120227130141.GA8980@do>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On Sun, Feb 26, 2012 at 06:11:30PM +0700, Nguyen Thai Ngoc Duy wrote:
>> "rev-list --objects" does check for blob existence, in finish_object().
>
> Eck.. I think "--quiet --verify-objects" becomes "--quiet --objects"
> because of this code:
>
> -- 8< --
> traverse_commit_list(&revs,
> quiet ? finish_commit : show_commit,
> quiet ? finish_object : show_object,
> &info);
> -- 8< --
>
> Unless that's intentional, shouldn't we apply this patch? --quiet's
> interfering with rev-list's business sounds weird to me.
Good thinking. Anything we are missing by calling finish_* other than
printing is a similar bug waiting to happen.
Can't we push the quiet bit in the info structure and have a single pair
of callback functions, so that we can make sure this kind of glitch would
never happen?
^ permalink raw reply
* [PATCH 1/2] post-receive-email: remove unused variable
From: mhagger @ 2012-02-27 18:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin P. Fleming, Andy Parkins, Michael Haggerty
In-Reply-To: <1330367650-23091-1-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
prep_for_email neither is passed a fourth argument nor uses it.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
contrib/hooks/post-receive-email | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index ba077c1..ac2e0ed 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -85,7 +85,6 @@ prep_for_email()
oldrev=$(git rev-parse $1)
newrev=$(git rev-parse $2)
refname="$3"
- maxlines=$4
# --- Interpret
# 0000->1234 (create)
--
1.7.9
^ permalink raw reply related
* [PATCH 2/2] post-receive-email: match up $LOGBEGIN..$LOGEND pairs correctly
From: mhagger @ 2012-02-27 18:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin P. Fleming, Andy Parkins, Michael Haggerty
In-Reply-To: <1330367650-23091-1-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
contrib/hooks/post-receive-email | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index ac2e0ed..01af9df 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -460,7 +460,7 @@ generate_delete_branch_email()
{
echo " was $oldrev"
echo ""
- echo $LOGEND
+ echo $LOGBEGIN
git show -s --pretty=oneline $oldrev
echo $LOGEND
}
@@ -560,7 +560,7 @@ generate_delete_atag_email()
{
echo " was $oldrev"
echo ""
- echo $LOGEND
+ echo $LOGBEGIN
git show -s --pretty=oneline $oldrev
echo $LOGEND
}
@@ -625,7 +625,7 @@ generate_delete_general_email()
{
echo " was $oldrev"
echo ""
- echo $LOGEND
+ echo $LOGBEGIN
git show -s --pretty=oneline $oldrev
echo $LOGEND
}
--
1.7.9
^ permalink raw reply related
* [PATCH 0/2] Trivial cleanups in the post-receive-email script
From: mhagger @ 2012-02-27 18:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kevin P. Fleming, Andy Parkins, Michael Haggerty
From: Michael Haggerty <mhagger@alum.mit.edu>
Should be self-explanatory.
Michael Haggerty (2):
post-receive-email: remove unused variable
post-receive-email: match up $LOGBEGIN..$LOGEND pairs correctly
contrib/hooks/post-receive-email | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
--
1.7.9
^ permalink raw reply
* Re: [PATCH 3/3] parse-options: remove PARSE_OPT_NEGHELP
From: Jeff King @ 2012-02-27 18:25 UTC (permalink / raw)
To: René Scharfe
Cc: git, Junio C Hamano, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <4F49336C.3000303@lsrfire.ath.cx>
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)
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.
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"),
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).
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.
-Peff
^ permalink raw reply
* Re: [PATCH v4] contrib: added git-diffall
From: Junio C Hamano @ 2012-02-27 18:17 UTC (permalink / raw)
To: Tim Henigan; +Cc: git, davvid, stefano.lattarini
In-Reply-To: <1330112937-7134-1-git-send-email-tim.henigan@gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> + -x|--e|--ex|--ext|--extc|--extcm|--extcmd)
> + if test $# == 1
That is not POSIX.
if test $# = 1
No need to resend; I'll fix it up locally.
^ permalink raw reply
* Re: [PATCH] Perform cheaper connectivity check when pack is used as medium
From: Junio C Hamano @ 2012-02-27 18:14 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330357191-32011-1-git-send-email-pclouds@gmail.com>
Please turn the existing "int quiet" that carries only one bit into a flag
word, instead of adding yet another int that is only used for one bit.
Other than that, the idea/approach feels sound.
Thanks.
^ permalink raw reply
* Re: [PATCH] branch: don't assume the merge filter ref exists
From: Junio C Hamano @ 2012-02-27 18:05 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Bernhard Reutner-Fischer, git
In-Reply-To: <1330355513-22351-1-git-send-email-cmn@elego.de>
Thanks.
^ permalink raw reply
* Re: [PATCH 05/11] show: use streaming interface for showing blobs
From: Junio C Hamano @ 2012-02-27 18:00 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330329315-11407-6-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/log.c | 9 ++++++++-
> t/t1050-large.sh | 2 +-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 7d1f6f8..4c4b17a 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -386,13 +386,20 @@ static int show_object(const unsigned char *sha1, int show_tag_object,
> {
> unsigned long size;
> enum object_type type;
> - char *buf = read_sha1_file(sha1, &type, &size);
> + char *buf;
> int offset = 0;
>
> + if (!show_tag_object) {
> + fflush(stdout);
> + return streaming_write_sha1(1, 0, sha1, OBJ_ANY, NULL);
> + }
> +
> + buf = read_sha1_file(sha1, &type, &size);
> if (!buf)
> return error(_("Could not read object %s"), sha1_to_hex(sha1));
>
> if (show_tag_object)
> + assert(type == OBJ_TAG);
> while (offset < size && buf[offset] != '\n') {
> int new_offset = offset + 1;
> while (new_offset < size && buf[new_offset++] != '\n')
Yuck.
The two callsites to this static function are to do BLOB to do TAG. And
after you start handing all the blob handling to streaming_write_sha1(),
there is no shared code between the two callers for this function.
So why not remove this function, create one show_blob_object() and the
other show_tag_object(), and update the callers to call the appropriate
one?
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index 39a3e77..66acb3b 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -118,7 +118,7 @@ test_expect_success 'cat-file a large file' '
> git cat-file blob :large1 >/dev/null
> '
>
> -test_expect_failure 'git-show a large file' '
> +test_expect_success 'git-show a large file' '
> git show :large1 >/dev/null
>
> '
^ permalink raw reply
* Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-
From: René Scharfe @ 2012-02-27 17:56 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, git, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <7v8vjob3ff.fsf@alter.siamese.dyndns.org>
Am 27.02.2012 18:18, schrieb Junio C Hamano:
> Thomas Rast<trast@inf.ethz.ch> writes:
>
>> Junio C Hamano<gitster@pobox.com> writes:
>>
>>> I would naïvely expect that it would be sufficient to update an existing
>>> definition for "--no-frotz" that uses PARSE_OPT_NONEG to instead define
>>> "--frotz" that by itself is a no-op, and "--no-frotz" would cause whatever
>>> the option currently means, with an update to the help text that says
>>> something to the effect that "--frotz by itself is meaningless and is
>>> always used as --no-frotz".
>>
>> Doesn't that last quote already answer your question?
>
> Yes, but only partly. I would agree with the awkwardness in
>
>> It would be rather awkward to see, in 'git apply -h',
>>
>> --add Also apply additions in the patch. This is the
>> default; use --no-add to disable it.
>
> but it feeels somewhat questionable that the solution to get this:
>
>>
>> Compare to the current concise wording
>>
>> --no-add ignore additions made by the patch
>
> is to define OPT_BOOL("no-add") that does not have any hint (other than
> the fact that the option name begins with 3 character "no-") that this is
> an already negated boolean and the "no-" negation can be removed.
The parser already knows that the prefix "no-" negates an option. It
currenmtly only applies this knowledge if that prefix is added, but not
if it is removed, which is inconsistent.
> This means an option "no-$foo" can never mean anything but "not foo". Not
> that we would have to or necessarily want to support an option to give the
> number of foo as --no-foo=47, as --num-foo=47 is a perfectly good spelling
> for such an option.
With the patch, you can define a --no-foo option that doesn't accept
--foo as its negation by specifying PARSE_OPT_NONEG. That would also
forbid --no-no-foo, though, but that's probably a good thing.
> If it were OPT_BOOL("no-foo", OPT_ISNEG | ...) that signals the parser
> that:
>
> - the option name is already negative;
> - the leading "no-" is to be removed to negate it; and
> - no extra leading "no-", i.e. "--no-no-foo", is accepted.
>
> I probably wouldn't have felt this uneasy iffiness.
Teaching the parser to understand that removal of the prefix "no-"
negates an option on top of its existing knowledge that adding it does
the same just adds the other side of the same coin, which was curiously
missing.
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.
René
^ permalink raw reply
* Re: [PATCH 03/11] cat-file: use streaming interface to print blobs
From: Junio C Hamano @ 2012-02-27 17:44 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330329315-11407-4-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/cat-file.c | 22 ++++++++++++++++++++++
> t/t1050-large.sh | 2 +-
> 2 files changed, 23 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 8ed501f..3f3b558 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -82,6 +82,24 @@ static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long
> write_or_die(1, cp, endp - cp);
> }
>
> +static int write_blob(const unsigned char *sha1)
> +{
> + unsigned char new_sha1[20];
> +
> + if (sha1_object_info(sha1, NULL) == OBJ_TAG) {
This smells bad. Why in the world could an API be sane if lets a caller
call "write_blob()" with something that can be a tag?
Both of your callsites call this function when (type == OBJ_BLOB), but the
"case 0:" arm in the large switch in cat_one_file() only checks "expected
type" which may not match the real type at all, so it is wrong to switch
on that in the first place. In addition, that call site alone needs to
deref tag to the requested/expected type.
This block does not belong to this function, but to only one of its
callers among two.
> + enum object_type type;
> + unsigned long size;
> + char *buffer = read_sha1_file(sha1, &type, &size);
> + if (memcmp(buffer, "object ", 7) ||
> + get_sha1_hex(buffer + 7, new_sha1))
> + die("%s not a valid tag", sha1_to_hex(sha1));
> + sha1 = new_sha1;
> + free(buffer);
> + }
> +
> + return streaming_write_sha1(1, 0, sha1, OBJ_BLOB, NULL);
I do not think your previous refactoring added a fall-back codepath to the
function you are calling here. In the original context, the caller of
streaming_write_entry() made sure that the blob is suitable for streaming
write by getting an istream, and called the function only when that is the
case. Blobs unsuitable for streaming (e.g. an deltified object in a pack)
were handled by the caller that decided not to call
streaming_write_entry() with the conventional "read to core and then write
it out" codepath.
And I do not think your updated caller in cat_one_file() is equipped to do
so at all.
So it looks to me that this patch totally breaks the cat-file. What am I
missing?
> +}
> +
> static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> {
> unsigned char sha1[20];
> @@ -127,6 +145,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> return cmd_ls_tree(2, ls_args, NULL);
> }
>
> + if (type == OBJ_BLOB)
> + return write_blob(sha1);
> buf = read_sha1_file(sha1, &type, &size);
> if (!buf)
> die("Cannot read object %s", obj_name);
> @@ -149,6 +169,8 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
> break;
>
> case 0:
> + if (type_from_string(exp_type) == OBJ_BLOB)
> + return write_blob(sha1);
> buf = read_object_with_reference(sha1, exp_type, &size, NULL);
> break;
>
> diff --git a/t/t1050-large.sh b/t/t1050-large.sh
> index f245e59..39a3e77 100755
> --- a/t/t1050-large.sh
> +++ b/t/t1050-large.sh
> @@ -114,7 +114,7 @@ test_expect_success 'hash-object' '
> git hash-object large1
> '
>
> -test_expect_failure 'cat-file a large file' '
> +test_expect_success 'cat-file a large file' '
> git cat-file blob :large1 >/dev/null
> '
^ 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