* [PATCH] send-email: document the --smtp-debug option
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-27 16:22 UTC (permalink / raw)
To: git, gitster; +Cc: greened, Zbigniew Jędrzejewski-Szmek
In-Reply-To: <87ehtiu5dg.fsf@smith.obbligato.org>
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?
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
~~~~~~~~~~
--
1.7.9.2.378.g4d260
^ permalink raw reply related
* [PATCH] grep -P: add tests for matching ^ and $
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-27 16:45 UTC (permalink / raw)
To: git, gitster; +Cc: michal.kiedrowicz, Zbigniew Jędrzejewski-Szmek
In-Reply-To: <7vlinpdxsu.fsf@alter.siamese.dyndns.org>
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.
Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
t/t7810-grep.sh | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 75f4716..d9ad633 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -47,6 +47,13 @@ test_expect_success setup '
echo vvv >t/v &&
mkdir t/a &&
echo vvv >t/a/v &&
+ {
+ echo "line without leading space1"
+ echo " line with leading space1"
+ echo " line with leading space2"
+ echo " line with leading space3"
+ echo "line without leading space2"
+ } >space &&
git add . &&
test_tick &&
git commit -m initial
@@ -893,4 +900,20 @@ test_expect_success 'mimic ack-grep --group' '
test_cmp expected actual
'
+cat >expected <<EOF
+space: line with leading space1
+space: line with leading space2
+space: line with leading space3
+EOF
+
+test_expect_success LIBPCRE 'grep -E "^ "' '
+ git grep -E "^ " space >actual &&
+ test_cmp expected actual
+'
+
+test_expect_success LIBPCRE 'grep -P "^ "' '
+ git grep -P "^ " space >actual &&
+ test_cmp expected actual
+'
+
test_done
--
1.7.9.2.396.ga883d.dirty
^ permalink raw reply related
* Re: [RFC/PATCH] Make git-{pull,rebase} no-tracking message friendlier
From: Carlos Martín Nieto @ 2012-02-27 17:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vhayh2t15.fsf@alter.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 1092 bytes --]
On Thu, 2012-02-23 at 12:28 -0800, Junio C Hamano wrote:
> Carlos Martín Nieto <cmn@elego.de> writes:
>
> > The current message is too long and at too low a level for anybody to
> > understand it if they don't know about the configuration format
> > already.
> >
> > Reformat it to show the commands a user would be expected to use,
> > instead of the contents of the configuration file.
> > ---
>
> Sounds like a change going in the right direction. I am unsure if it is a
> good idea to remove "See git-config...", but otherwise I like the updated
> text much better.
We're already telling them to go look at the git-pull or git-rebase
manpages, where there's already a lot of information and adding another
RTFM seemed a bit unfriendly (and not that useful, as they'd be
manipulating the config via other commands and shouldn't need to care
about config that much)
>
> But of course I am not the target audience, so let's see what we hear from
> others.
Sure, I'll see if I can come up with some better wording and send a
signed-off patch.
cmn
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: [PATCH 2/3] parse-options: allow positivation of options starting, with no-
From: Junio C Hamano @ 2012-02-27 17:18 UTC (permalink / raw)
To: Thomas Rast
Cc: René Scharfe, git, Bert Wesarg, Geoffrey Irving,
Johannes Schindelin, Pierre Habouzit
In-Reply-To: <87d390smpa.fsf@thomas.inf.ethz.ch>
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.
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.
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.
But it is just a minor issue.
^ 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 17:29 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1330329315-11407-3-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>
> ---
> cache.h | 3 +++
> entry.c | 39 ++++++++++++++++++++++++++-------------
> 2 files changed, 29 insertions(+), 13 deletions(-)
It was the goal of the original streaming output topic to helping more
callers stream the data out directly from the object store in order to
reduce memory pressure, and this series is very much in line with its
spirit.
The static version of streaming_write_entry() in entry.c was very specific
to writing out an index entry out to the working tree, and it made perfect
sense to have the function in that file, but its interface was limited to
the original context the function was used in.
The whole point of your refactoring in this patch is to make it available
for callers outside that original context; e.g. archive that finds blob
SHA-1 from a tree and writes the blob out to its standard output. They
should not have to work with an API that takes a cache-entry and writes to
a working tree file. And your result is much more generic.
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.
^ permalink raw reply
* [PATCH] git-p4: missing she-bang line in t9804 confuses prove
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-27 17:40 UTC (permalink / raw)
To: git, gitster; +Cc: luke, Zbigniew Jędrzejewski-Szmek
Without the magic line, prove shows lots and lots of errors:
% prove ./t9804-git-p4-label.sh
./t9804-git-p4-label.sh .. syntax error at ./t9804-git-p4-label.sh line 3, near ". ."
...
When #!/bin/sh is added, tests are skipped (I have no p4d).
Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
t/t9804-git-p4-label.sh | 2 ++
1 file changed, 2 insertions(+)
diff --git a/t/t9804-git-p4-label.sh b/t/t9804-git-p4-label.sh
index 80d01ea..a9e04ef 100755
--- a/t/t9804-git-p4-label.sh
+++ b/t/t9804-git-p4-label.sh
@@ -1,3 +1,5 @@
+#!/bin/sh
+
test_description='git-p4 p4 label tests'
. ./lib-git-p4.sh
--
1.7.9.2.396.ga883d.dirty
^ permalink raw reply related
* [PATCH 1/2] CodingGuidelines: Add a note about spaces after redirection
From: Tim Henigan @ 2012-02-24 21: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>
---
Documentation/CodingGuidelines | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 4830086..a4ffe7c 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -35,6 +35,11 @@ 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'
+
- 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
* [PATCH 2/2] CodingGuidelines: Add note forbidding use of 'which' in shell scripts
From: Tim Henigan @ 2012-02-24 21:12 UTC (permalink / raw)
To: git, gitster; +Cc: tim.henigan
In-Reply-To: <1330117921-8257-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.
Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
Documentation/CodingGuidelines | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
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.
+
- We use POSIX compliant parameter substitutions and avoid bashisms;
namely:
--
1.7.9.1
^ permalink raw reply related
* 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
* 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 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] 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] 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 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 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
* [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
* [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 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
* 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
* 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: [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 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 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 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 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
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