Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes
On 28 June 2017 at 14:14, miguel torroja wrote: > Thanks Luke, > > regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT > should display error), for me it's very weird too as it doesn't seem > to be related to this particular change, as the patch changes are not > exercised with that test. I had a look at this. The problem is that the old code uses p4_read_pipe_lines() which calls sys.exit() if the subprocess fails. But the new code calls p4CmdList() which has does error handling by setting "p4ExitCode" to a non-zero value in the returned dictionary. I think if you just check for that case, the test will then pass. > > The test 21 in t9807 was precisely the new test added to test the > change (it was passing with local setup), the test log is truncated > before the output of test 21 in t9807 but I'm afraid I'm not very > familiar with Travis, so maybe I'm missing something. Is there a way > to have the full logs or they are always truncated after some number > of lines? For me, t9807 is working fine. > > I think you get an error with git diff --check because I added spaces > after a tab, but those spaces are intentional, the tabs are for the > "<<-EOF" and spaces are for the "p4 triggers" specificiation. OK. Luke
[PATCH 2/2] hashmap: migrate documentation from Documentation/technical into header
While at it, clarify the use of `key`, `keydata`, `entry_or_key` as well as documenting the new data pointer for the compare function. Signed-off-by: Stefan Beller --- Documentation/technical/api-hashmap.txt | 309 hashmap.h | 249 +++-- 2 files changed, 231 insertions(+), 327 deletions(-) delete mode 100644 Documentation/technical/api-hashmap.txt diff --git a/Documentation/technical/api-hashmap.txt b/Documentation/technical/api-hashmap.txt deleted file mode 100644 index ccc634bbd7..00 --- a/Documentation/technical/api-hashmap.txt +++ /dev/null @@ -1,309 +0,0 @@ -hashmap API -=== - -The hashmap API is a generic implementation of hash-based key-value mappings. - -Data Structures - -`struct hashmap`:: - - The hash table structure. Members can be used as follows, but should - not be modified directly: -+ -The `size` member keeps track of the total number of entries (0 means the -hashmap is empty). -+ -`tablesize` is the allocated size of the hash table. A non-0 value indicates -that the hashmap is initialized. It may also be useful for statistical purposes -(i.e. `size / tablesize` is the current load factor). -+ -`cmpfn` stores the comparison function specified in `hashmap_init()`. In -advanced scenarios, it may be useful to change this, e.g. to switch between -case-sensitive and case-insensitive lookup. -+ -When `disallow_rehash` is set, automatic rehashes are prevented during inserts -and deletes. - -`struct hashmap_entry`:: - - An opaque structure representing an entry in the hash table, which must - be used as first member of user data structures. Ideally it should be - followed by an int-sized member to prevent unused memory on 64-bit - systems due to alignment. -+ -The `hash` member is the entry's hash code and the `next` member points to the -next entry in case of collisions (i.e. if multiple entries map to the same -bucket). - -`struct hashmap_iter`:: - - An iterator structure, to be used with hashmap_iter_* functions. - -Types -- - -`int (*hashmap_cmp_fn)(const void *entry, const void *entry_or_key, const void *keydata)`:: - - User-supplied function to test two hashmap entries for equality. Shall - return 0 if the entries are equal. -+ -This function is always called with non-NULL `entry` / `entry_or_key` -parameters that have the same hash code. When looking up an entry, the `key` -and `keydata` parameters to hashmap_get and hashmap_remove are always passed -as second and third argument, respectively. Otherwise, `keydata` is NULL. - -Functions -- - -`unsigned int strhash(const char *buf)`:: -`unsigned int strihash(const char *buf)`:: -`unsigned int memhash(const void *buf, size_t len)`:: -`unsigned int memihash(const void *buf, size_t len)`:: -`unsigned int memihash_cont(unsigned int hash_seed, const void *buf, size_t len)`:: - - Ready-to-use hash functions for strings, using the FNV-1 algorithm (see - http://www.isthe.com/chongo/tech/comp/fnv). -+ -`strhash` and `strihash` take 0-terminated strings, while `memhash` and -`memihash` operate on arbitrary-length memory. -+ -`strihash` and `memihash` are case insensitive versions. -+ -`memihash_cont` is a variant of `memihash` that allows a computation to be -continued with another chunk of data. - -`unsigned int sha1hash(const unsigned char *sha1)`:: - - Converts a cryptographic hash (e.g. SHA-1) into an int-sized hash code - for use in hash tables. Cryptographic hashes are supposed to have - uniform distribution, so in contrast to `memhash()`, this just copies - the first `sizeof(int)` bytes without shuffling any bits. Note that - the results will be different on big-endian and little-endian - platforms, so they should not be stored or transferred over the net. - -`void hashmap_init(struct hashmap *map, hashmap_cmp_fn equals_function, size_t initial_size)`:: - - Initializes a hashmap structure. -+ -`map` is the hashmap to initialize. -+ -The `equals_function` can be specified to compare two entries for equality. -If NULL, entries are considered equal if their hash codes are equal. -+ -If the total number of entries is known in advance, the `initial_size` -parameter may be used to preallocate a sufficiently large table and thus -prevent expensive resizing. If 0, the table is dynamically resized. - -`void hashmap_free(struct hashmap *map, int free_entries)`:: - - Frees a hashmap structure and allocated memory. -+ -`map` is the hashmap to free. -+ -If `free_entries` is true, each hashmap_entry in the map is freed as well -(using stdlib's free()). - -`void hashmap_entry_init(void *entry, unsigned int hash)`:: - - Initializes a hashmap_entry structure. -+ -`entry` points to the entry to initialize. -+ -`hash` is the hash code of the entry. -+ -The hashmap_entry structure does not hold references to exte
[PATCH 1/2] hashmap.h: compare function has access to a data field
When using the hashmap a common need is to have access to arbitrary data
in the compare function. A couple of times we abuse the keydata field
to pass in the data needed. This happens for example in patch-ids.c.
This patch changes the function signature of the compare function
to have one more void pointer available. The pointer given for each
invocation of the compare function must be defined in the init function
of the hashmap and is just passed through.
Documentation of this new feature is deferred to a later patch.
While at it improve the naming of the fields of all compare functions used
by hashmaps by ensuring unused parameters are prefixed with 'unused_' and
naming the parameters what they are (instead of 'unused' make it
'unused_keydata').
Signed-off-by: Stefan Beller
---
attr.c | 4 ++--
builtin/describe.c | 6 --
builtin/difftool.c | 20
builtin/fast-export.c | 5 +++--
config.c| 7 +--
convert.c | 3 ++-
diffcore-rename.c | 2 +-
hashmap.c | 17 -
hashmap.h | 9 ++---
name-hash.c | 12
oidset.c| 5 +++--
patch-ids.c | 6 --
refs.c | 4 ++--
remote.c| 7 +--
sha1_file.c | 5 +++--
sub-process.c | 5 +++--
sub-process.h | 6 --
submodule-config.c | 10 ++
t/helper/test-hashmap.c | 15 ++-
19 files changed, 95 insertions(+), 53 deletions(-)
diff --git a/attr.c b/attr.c
index 37454999d2..2f51417675 100644
--- a/attr.c
+++ b/attr.c
@@ -78,7 +78,7 @@ struct attr_hash_entry {
/* attr_hashmap comparison function */
static int attr_hash_entry_cmp(const struct attr_hash_entry *a,
const struct attr_hash_entry *b,
- void *unused)
+ void *unused_keydata, void *unused_data)
{
return (a->keylen != b->keylen) || strncmp(a->key, b->key, a->keylen);
}
@@ -86,7 +86,7 @@ static int attr_hash_entry_cmp(const struct attr_hash_entry
*a,
/* Initialize an 'attr_hashmap' object */
static void attr_hashmap_init(struct attr_hashmap *map)
{
- hashmap_init(&map->map, (hashmap_cmp_fn) attr_hash_entry_cmp, 0);
+ hashmap_init(&map->map, (hashmap_cmp_fn) attr_hash_entry_cmp, NULL, 0);
}
/*
diff --git a/builtin/describe.c b/builtin/describe.c
index 70eb144608..a6c5a969a0 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -55,7 +55,9 @@ static const char *prio_names[] = {
};
static int commit_name_cmp(const struct commit_name *cn1,
- const struct commit_name *cn2, const void *peeled)
+ const struct commit_name *cn2,
+ const void *peeled,
+ const void *unused_data)
{
return oidcmp(&cn1->peeled, peeled ? peeled : &cn2->peeled);
}
@@ -501,7 +503,7 @@ int cmd_describe(int argc, const char **argv, const char
*prefix)
return cmd_name_rev(args.argc, args.argv, prefix);
}
- hashmap_init(&names, (hashmap_cmp_fn) commit_name_cmp, 0);
+ hashmap_init(&names, (hashmap_cmp_fn) commit_name_cmp, NULL, 0);
for_each_rawref(get_name, NULL);
if (!names.size && !always)
die(_("No names found, cannot describe anything."));
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 9199227f6e..80786a95ab 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -131,7 +131,9 @@ struct working_tree_entry {
};
static int working_tree_entry_cmp(struct working_tree_entry *a,
- struct working_tree_entry *b, void *keydata)
+ struct working_tree_entry *b,
+ void *unused_keydata,
+ void *unused_data)
{
return strcmp(a->path, b->path);
}
@@ -146,7 +148,8 @@ struct pair_entry {
const char path[FLEX_ARRAY];
};
-static int pair_cmp(struct pair_entry *a, struct pair_entry *b, void *keydata)
+static int pair_cmp(struct pair_entry *a, struct pair_entry *b,
+ void *unused_keydata, void *unused_data)
{
return strcmp(a->path, b->path);
}
@@ -174,7 +177,8 @@ struct path_entry {
char path[FLEX_ARRAY];
};
-static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void
*key)
+static int path_entry_cmp(struct path_entry *a, struct path_entry *b,
+ void *key, void *unused_data)
{
return strcmp(a->path, key ? key : b->path);
}
@@ -367,9 +371,9 @@ static int run_dir_diff(const char *extcmd, int symlinks,
const char *prefix,
wtdir_len = wtdir.len;
hashmap_init(&working_tree_dups,
-(hashmap_cmp_fn)working_tree_entry_cmp, 0);
- hashmap_init(&submodules, (hashmap_cmp_fn)pair_cmp, 0);
-
[PATCH 0/2] Introduce data field in hashmap and migrate docs to header
https://public-inbox.org/git/xmqqpodnvmmw@gitster.mtv.corp.google.com/ for context why we need a new data field. Implement that. Once upon a time we had a long discussion where to put documentation best. The answer was header files as there documentation has less chance to become stale and be out of date. Improve the docs by * migrating them to the header * clarifying how the compare function is to be used * how the arguments to hashmap_get/remove should be used. Thanks, Stefan Stefan Beller (2): hashmap.h: compare function has access to a data field hashmap: migrate documentation from Documentation/technical into header Documentation/technical/api-hashmap.txt | 309 attr.c | 4 +- builtin/describe.c | 6 +- builtin/difftool.c | 20 ++- builtin/fast-export.c | 5 +- config.c| 7 +- convert.c | 3 +- diffcore-rename.c | 2 +- hashmap.c | 17 +- hashmap.h | 256 +++--- name-hash.c | 12 +- oidset.c| 5 +- patch-ids.c | 6 +- refs.c | 4 +- remote.c| 7 +- sha1_file.c | 5 +- sub-process.c | 5 +- sub-process.h | 6 +- submodule-config.c | 10 +- t/helper/test-hashmap.c | 15 +- 20 files changed, 325 insertions(+), 379 deletions(-) delete mode 100644 Documentation/technical/api-hashmap.txt -- 2.13.0.31.g9b732c453e
Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
Ævar Arnfjörð Bjarmason writes: > I get exactly the same thing as you do below when following these > steps. So it seems your patch in > 2122b01f-7627-cd1b-c7df-751c076f9...@web.de is just fine as-is and I > just screwed something up when testing this. > > Sorry about the noise. Since this works your original patch is obviously > preferrable since it's not duplicating the rule. OK. Unfortunately I screwed up and merged the revert already to 'next'. So I'll queue René's original again to 'next' and we'll only have one rule at the end. Thanks, both.
Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
Am 29.06.2017 um 00:21 schrieb René Scharfe: $ git am # pasted my email Applying: coccinelle: add a rule to make "expression" code use FREE_AND_NULL() # results in commit message with scissor line, interesting.. "git am --scissors" commits with the correct message. I'll do a "git config --local --add mailinfo.scissors true" real quick.. René
Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
On Thu, Jun 29, 2017 at 12:30 AM, René Scharfe wrote:
> Am 29.06.2017 um 00:21 schrieb René Scharfe:
>>
>> Am 28.06.2017 um 23:39 schrieb Ævar Arnfjörð Bjarmason:
>>>
>>>
>>> On Sun, Jun 25 2017, René Scharfe jotted:
>>>
Am 16.06.2017 um 21:43 schrieb Junio C Hamano:
>
> Ævar Arnfjörð Bjarmason writes:
>
>> A follow-up to the existing "type" rule added in an earlier
>> change. This catches some occurrences that are missed by the previous
>> rule.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason
>> ---
>
>
> Hmph, I wonder if the "type" thing is really needed. Over there,
> "ptr" is an expression and we can find "free(ptr); ptr = NULL" with
> the rule in this patch already, no?
Indeed. How about this on top of master?
-- >8 --
Subject: [PATCH] coccinelle: polish FREE_AND_NULL rules
There are two rules for using FREE_AND_NULL in free.cocci, one for
pointer types and one for expressions. Both cause coccinelle to remove
empty lines and even newline characters between replacements for some
reason; consecutive "free(x);/x=NULL;" sequences end up as multiple
FREE_AND_NULL calls on the same time.
Remove the type rule, as the expression rule already covers it, and
rearrange the lines of the latter to place the addition of FREE_AND_NULL
between the removals, which causes coccinelle to leave surrounding
whitespace untouched.
Signed-off-by: Rene Scharfe
---
contrib/coccinelle/free.cocci | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/contrib/coccinelle/free.cocci
b/contrib/coccinelle/free.cocci
index f2d97e755b..4490069df9 100644
--- a/contrib/coccinelle/free.cocci
+++ b/contrib/coccinelle/free.cocci
@@ -11,16 +11,8 @@ expression E;
free(E);
@@
-type T;
-T *ptr;
-@@
-- free(ptr);
-- ptr = NULL;
-+ FREE_AND_NULL(ptr);
-
-@@
expression E;
@@
- free(E);
-- E = NULL;
+ FREE_AND_NULL(E);
+- E = NULL;
>>>
>>>
>>> Late reply, sorry. What version of coccinelle are you running? I have
>>> 1.0.4 (from Debian) and can't get this to produce the same results as
>>> what I have.
>>>
>>> On top of next, I did:
>>>
>>> Revert "*.[ch] refactoring: make use of the FREE_AND_NULL()
>>> macro"
>>> Revert "coccinelle: make use of the "expression"
>>> FREE_AND_NULL() rule"
>>> Revert "coccinelle: make use of the "type" FREE_AND_NULL()
>>> rule"
>>>
>>> And then generated the patch as usual with `make coccicheck`, and
>>> applied it. This has your change.
>>>
>>> I then re-applied the manual "*.[ch] refactoring" changes
>>>
>>> This results in this diff with next:
>>>
>>> $ git diff --stat origin/next.. -- '*.[ch]'
>>>builtin/am.c | 3 ++-
>>>builtin/clean.c | 6 --
>>>builtin/config.c | 6 --
>>>builtin/index-pack.c | 6 --
>>>builtin/pack-objects.c | 12
>>>builtin/unpack-objects.c | 3 ++-
>>>fast-import.c| 6 --
>>>http-push.c | 24
>>>http.c | 15 ++-
>>>imap-send.c | 3 ++-
>>>ref-filter.c | 3 ++-
>>>refs/files-backend.c | 3 ++-
>>>remote-testsvn.c | 3 ++-
>>>sequencer.c | 3 ++-
>>>sha1-array.c | 3 ++-
>>>sha1_file.c | 3 ++-
>>>transport-helper.c | 27 ++-
>>>transport.c | 3 ++-
>>>tree-diff.c | 6 --
>>>tree.c | 3 ++-
>>>20 files changed, 94 insertions(+), 47 deletions(-)
>>>
>>> These are all cases where we now miss things that should use
>>> FREE_AND_NULL(), e.g.:
>>>
>>> diff --git a/builtin/am.c b/builtin/am.c
>>> index c973bd96dc..2f89338ed7 100644
>>> --- a/builtin/am.c
>>> +++ b/builtin/am.c
>>> @@ -484,7 +484,8 @@ static int run_applypatch_msg_hook(struct
>>> am_state *state)
>>> ret = run_hook_le(NULL, "applypatch-msg", am_path(state,
>>> "final-commit"), NULL);
>>>
>>> if (!ret) {
>>> - FREE_AND_NULL(state->msg);
>>> + free(state->msg);
>>> + state->msg = NULL;
>>> if (read_commit_msg(state) < 0)
>>> die(_("'%s' was deleted by the
>>> applypatch-msg hook"),
>>> am_path(state, "final-commit"));
>>>
>>> So it looks to me like removing the "type T" rule breaks a lot of
>>> things, but that the change you made to the expression rule is good, and
>>> we should do that for the "type" rule
Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
Am 29.06.2017 um 00:21 schrieb René Scharfe:
Am 28.06.2017 um 23:39 schrieb Ævar Arnfjörð Bjarmason:
On Sun, Jun 25 2017, René Scharfe jotted:
Am 16.06.2017 um 21:43 schrieb Junio C Hamano:
Ævar Arnfjörð Bjarmason writes:
A follow-up to the existing "type" rule added in an earlier
change. This catches some occurrences that are missed by the previous
rule.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
Hmph, I wonder if the "type" thing is really needed. Over there,
"ptr" is an expression and we can find "free(ptr); ptr = NULL" with
the rule in this patch already, no?
Indeed. How about this on top of master?
-- >8 --
Subject: [PATCH] coccinelle: polish FREE_AND_NULL rules
There are two rules for using FREE_AND_NULL in free.cocci, one for
pointer types and one for expressions. Both cause coccinelle to remove
empty lines and even newline characters between replacements for some
reason; consecutive "free(x);/x=NULL;" sequences end up as multiple
FREE_AND_NULL calls on the same time.
Remove the type rule, as the expression rule already covers it, and
rearrange the lines of the latter to place the addition of FREE_AND_NULL
between the removals, which causes coccinelle to leave surrounding
whitespace untouched.
Signed-off-by: Rene Scharfe
---
contrib/coccinelle/free.cocci | 10 +-
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
index f2d97e755b..4490069df9 100644
--- a/contrib/coccinelle/free.cocci
+++ b/contrib/coccinelle/free.cocci
@@ -11,16 +11,8 @@ expression E;
free(E);
@@
-type T;
-T *ptr;
-@@
-- free(ptr);
-- ptr = NULL;
-+ FREE_AND_NULL(ptr);
-
-@@
expression E;
@@
- free(E);
-- E = NULL;
+ FREE_AND_NULL(E);
+- E = NULL;
Late reply, sorry. What version of coccinelle are you running? I have
1.0.4 (from Debian) and can't get this to produce the same results as
what I have.
On top of next, I did:
Revert "*.[ch] refactoring: make use of the FREE_AND_NULL() macro"
Revert "coccinelle: make use of the "expression" FREE_AND_NULL() rule"
Revert "coccinelle: make use of the "type" FREE_AND_NULL() rule"
And then generated the patch as usual with `make coccicheck`, and
applied it. This has your change.
I then re-applied the manual "*.[ch] refactoring" changes
This results in this diff with next:
$ git diff --stat origin/next.. -- '*.[ch]'
builtin/am.c | 3 ++-
builtin/clean.c | 6 --
builtin/config.c | 6 --
builtin/index-pack.c | 6 --
builtin/pack-objects.c | 12
builtin/unpack-objects.c | 3 ++-
fast-import.c| 6 --
http-push.c | 24
http.c | 15 ++-
imap-send.c | 3 ++-
ref-filter.c | 3 ++-
refs/files-backend.c | 3 ++-
remote-testsvn.c | 3 ++-
sequencer.c | 3 ++-
sha1-array.c | 3 ++-
sha1_file.c | 3 ++-
transport-helper.c | 27 ++-
transport.c | 3 ++-
tree-diff.c | 6 --
tree.c | 3 ++-
20 files changed, 94 insertions(+), 47 deletions(-)
These are all cases where we now miss things that should use
FREE_AND_NULL(), e.g.:
diff --git a/builtin/am.c b/builtin/am.c
index c973bd96dc..2f89338ed7 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -484,7 +484,8 @@ static int run_applypatch_msg_hook(struct am_state
*state)
ret = run_hook_le(NULL, "applypatch-msg", am_path(state,
"final-commit"), NULL);
if (!ret) {
- FREE_AND_NULL(state->msg);
+ free(state->msg);
+ state->msg = NULL;
if (read_commit_msg(state) < 0)
die(_("'%s' was deleted by the applypatch-msg
hook"),
am_path(state, "final-commit"));
So it looks to me like removing the "type T" rule breaks a lot of
things, but that the change you made to the expression rule is good, and
we should do that for the "type" rule as well. Your commit says the
"expression rule already covers it", but this doesn't seem to be the
case at all.
As an aside: Junio, did you mean to apply f8bb4631fb to next this way?
Looks like a mis-applied scissor commit.
I can't reproduce that strange result with master, but get a scissors
line into the commit message as well. The diff at the end looks
reasonable (contains just the manual stuff). Here's what I did:
On top of next it looks basically the same for me, only difference
being several new converted cases in repository.c.
Did you commit before diff?
René
Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
On Wed, Jun 28 2017, René Scharfe jotted:
> Am 28.06.2017 um 23:39 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Sun, Jun 25 2017, René Scharfe jotted:
>>
>>> Am 16.06.2017 um 21:43 schrieb Junio C Hamano:
Ævar Arnfjörð Bjarmason writes:
> A follow-up to the existing "type" rule added in an earlier
> change. This catches some occurrences that are missed by the previous
> rule.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason
> ---
Hmph, I wonder if the "type" thing is really needed. Over there,
"ptr" is an expression and we can find "free(ptr); ptr = NULL" with
the rule in this patch already, no?
>>>
>>> Indeed. How about this on top of master?
>>>
>>> -- >8 --
>>> Subject: [PATCH] coccinelle: polish FREE_AND_NULL rules
>>>
>>> There are two rules for using FREE_AND_NULL in free.cocci, one for
>>> pointer types and one for expressions. Both cause coccinelle to remove
>>> empty lines and even newline characters between replacements for some
>>> reason; consecutive "free(x);/x=NULL;" sequences end up as multiple
>>> FREE_AND_NULL calls on the same time.
>>>
>>> Remove the type rule, as the expression rule already covers it, and
>>> rearrange the lines of the latter to place the addition of FREE_AND_NULL
>>> between the removals, which causes coccinelle to leave surrounding
>>> whitespace untouched.
>>>
>>> Signed-off-by: Rene Scharfe
>>> ---
>>> contrib/coccinelle/free.cocci | 10 +-
>>> 1 file changed, 1 insertion(+), 9 deletions(-)
>>>
>>> diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
>>> index f2d97e755b..4490069df9 100644
>>> --- a/contrib/coccinelle/free.cocci
>>> +++ b/contrib/coccinelle/free.cocci
>>> @@ -11,16 +11,8 @@ expression E;
>>> free(E);
>>>
>>> @@
>>> -type T;
>>> -T *ptr;
>>> -@@
>>> -- free(ptr);
>>> -- ptr = NULL;
>>> -+ FREE_AND_NULL(ptr);
>>> -
>>> -@@
>>> expression E;
>>> @@
>>> - free(E);
>>> -- E = NULL;
>>> + FREE_AND_NULL(E);
>>> +- E = NULL;
>>
>> Late reply, sorry. What version of coccinelle are you running? I have
>> 1.0.4 (from Debian) and can't get this to produce the same results as
>> what I have.
>>
>> On top of next, I did:
>>
>> Revert "*.[ch] refactoring: make use of the FREE_AND_NULL() macro"
>> Revert "coccinelle: make use of the "expression" FREE_AND_NULL()
>> rule"
>> Revert "coccinelle: make use of the "type" FREE_AND_NULL() rule"
>>
>> And then generated the patch as usual with `make coccicheck`, and
>> applied it. This has your change.
>>
>> I then re-applied the manual "*.[ch] refactoring" changes
>>
>> This results in this diff with next:
>>
>> $ git diff --stat origin/next.. -- '*.[ch]'
>> builtin/am.c | 3 ++-
>> builtin/clean.c | 6 --
>> builtin/config.c | 6 --
>> builtin/index-pack.c | 6 --
>> builtin/pack-objects.c | 12
>> builtin/unpack-objects.c | 3 ++-
>> fast-import.c| 6 --
>> http-push.c | 24
>> http.c | 15 ++-
>> imap-send.c | 3 ++-
>> ref-filter.c | 3 ++-
>> refs/files-backend.c | 3 ++-
>> remote-testsvn.c | 3 ++-
>> sequencer.c | 3 ++-
>> sha1-array.c | 3 ++-
>> sha1_file.c | 3 ++-
>> transport-helper.c | 27 ++-
>> transport.c | 3 ++-
>> tree-diff.c | 6 --
>> tree.c | 3 ++-
>> 20 files changed, 94 insertions(+), 47 deletions(-)
>>
>> These are all cases where we now miss things that should use
>> FREE_AND_NULL(), e.g.:
>>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index c973bd96dc..2f89338ed7 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -484,7 +484,8 @@ static int run_applypatch_msg_hook(struct am_state
>> *state)
>> ret = run_hook_le(NULL, "applypatch-msg", am_path(state,
>> "final-commit"), NULL);
>>
>> if (!ret) {
>> - FREE_AND_NULL(state->msg);
>> + free(state->msg);
>> + state->msg = NULL;
>> if (read_commit_msg(state) < 0)
>> die(_("'%s' was deleted by the applypatch-msg
>> hook"),
>> am_path(state, "final-commit"));
>>
>> So it looks to me like removing the "type T" rule breaks a lot of
>> things, but that the change you made to the expression rule is good, and
>> we should do that for the "type" rule as well. Your commit says the
>> "expression rule already covers it", but this doesn't seem to be the
>> case at all.
>>
>> As an aside: Junio, did you mean to apply f8bb4631fb to next this way?
>> Looks like a mis-applied scissor commit.
>
> I can't reproduce that st
IP/ID Offer
Dear Sir/Ma I am brokering an Investment deal in your country on behalf of a client. This deal will involve the eventual investment of US$50M ( Fifty Million United States Dollars) within your country. I want to offer you an opportunity to be part of this profitable and life time business opportunity; offering you a partnership, so as to allow you manage and invest the funds in your country within the time frame of the investment/partnership scheme. I've kept this proposal brief, since I cannot determine if this email is still functional and also your willingness to be part of this deal. If you are Interested in this offer of partnership, please respond and provide your current contact details, so I can send to you details of this offer. Your expedient response will be appreciated. Mich Brown
Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
Am 28.06.2017 um 23:39 schrieb Ævar Arnfjörð Bjarmason:
>
> On Sun, Jun 25 2017, René Scharfe jotted:
>
>> Am 16.06.2017 um 21:43 schrieb Junio C Hamano:
>>> Ævar Arnfjörð Bjarmason writes:
>>>
A follow-up to the existing "type" rule added in an earlier
change. This catches some occurrences that are missed by the previous
rule.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
>>>
>>> Hmph, I wonder if the "type" thing is really needed. Over there,
>>> "ptr" is an expression and we can find "free(ptr); ptr = NULL" with
>>> the rule in this patch already, no?
>>
>> Indeed. How about this on top of master?
>>
>> -- >8 --
>> Subject: [PATCH] coccinelle: polish FREE_AND_NULL rules
>>
>> There are two rules for using FREE_AND_NULL in free.cocci, one for
>> pointer types and one for expressions. Both cause coccinelle to remove
>> empty lines and even newline characters between replacements for some
>> reason; consecutive "free(x);/x=NULL;" sequences end up as multiple
>> FREE_AND_NULL calls on the same time.
>>
>> Remove the type rule, as the expression rule already covers it, and
>> rearrange the lines of the latter to place the addition of FREE_AND_NULL
>> between the removals, which causes coccinelle to leave surrounding
>> whitespace untouched.
>>
>> Signed-off-by: Rene Scharfe
>> ---
>> contrib/coccinelle/free.cocci | 10 +-
>> 1 file changed, 1 insertion(+), 9 deletions(-)
>>
>> diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
>> index f2d97e755b..4490069df9 100644
>> --- a/contrib/coccinelle/free.cocci
>> +++ b/contrib/coccinelle/free.cocci
>> @@ -11,16 +11,8 @@ expression E;
>> free(E);
>>
>> @@
>> -type T;
>> -T *ptr;
>> -@@
>> -- free(ptr);
>> -- ptr = NULL;
>> -+ FREE_AND_NULL(ptr);
>> -
>> -@@
>> expression E;
>> @@
>> - free(E);
>> -- E = NULL;
>> + FREE_AND_NULL(E);
>> +- E = NULL;
>
> Late reply, sorry. What version of coccinelle are you running? I have
> 1.0.4 (from Debian) and can't get this to produce the same results as
> what I have.
>
> On top of next, I did:
>
> Revert "*.[ch] refactoring: make use of the FREE_AND_NULL() macro"
> Revert "coccinelle: make use of the "expression" FREE_AND_NULL()
> rule"
> Revert "coccinelle: make use of the "type" FREE_AND_NULL() rule"
>
> And then generated the patch as usual with `make coccicheck`, and
> applied it. This has your change.
>
> I then re-applied the manual "*.[ch] refactoring" changes
>
> This results in this diff with next:
>
> $ git diff --stat origin/next.. -- '*.[ch]'
> builtin/am.c | 3 ++-
> builtin/clean.c | 6 --
> builtin/config.c | 6 --
> builtin/index-pack.c | 6 --
> builtin/pack-objects.c | 12
> builtin/unpack-objects.c | 3 ++-
> fast-import.c| 6 --
> http-push.c | 24
> http.c | 15 ++-
> imap-send.c | 3 ++-
> ref-filter.c | 3 ++-
> refs/files-backend.c | 3 ++-
> remote-testsvn.c | 3 ++-
> sequencer.c | 3 ++-
> sha1-array.c | 3 ++-
> sha1_file.c | 3 ++-
> transport-helper.c | 27 ++-
> transport.c | 3 ++-
> tree-diff.c | 6 --
> tree.c | 3 ++-
> 20 files changed, 94 insertions(+), 47 deletions(-)
>
> These are all cases where we now miss things that should use
> FREE_AND_NULL(), e.g.:
>
> diff --git a/builtin/am.c b/builtin/am.c
> index c973bd96dc..2f89338ed7 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -484,7 +484,8 @@ static int run_applypatch_msg_hook(struct am_state
> *state)
> ret = run_hook_le(NULL, "applypatch-msg", am_path(state,
> "final-commit"), NULL);
>
> if (!ret) {
> - FREE_AND_NULL(state->msg);
> + free(state->msg);
> + state->msg = NULL;
> if (read_commit_msg(state) < 0)
> die(_("'%s' was deleted by the applypatch-msg
> hook"),
> am_path(state, "final-commit"));
>
> So it looks to me like removing the "type T" rule breaks a lot of
> things, but that the change you made to the expression rule is good, and
> we should do that for the "type" rule as well. Your commit says the
> "expression rule already covers it", but this doesn't seem to be the
> case at all.
>
> As an aside: Junio, did you mean to apply f8bb4631fb to next this way?
> Looks like a mis-applied scissor commit.
I can't reproduce that strange result with master, but get a scissors
line into the commit message as well. The diff at the end looks
reasonable (contains just the manual stuff). Here's wha
Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
On Wed, Jun 28 2017, Junio C. Hamano jotted:
> Ævar Arnfjörð Bjarmason writes:
>
>> Late reply, sorry. What version of coccinelle are you running? I have
>> 1.0.4 (from Debian) and can't get this to produce the same results as
>> what I have.
>>
>> On top of next, I did:
>>
>> Revert "*.[ch] refactoring: make use of the FREE_AND_NULL() macro"
>> Revert "coccinelle: make use of the "expression" FREE_AND_NULL()
>> rule"
>> Revert "coccinelle: make use of the "type" FREE_AND_NULL() rule"
>>
>> And then generated the patch as usual with `make coccicheck`, and
>> applied it. This has your change.
>>
>> I then re-applied the manual "*.[ch] refactoring" changes
>>
>> This results in this diff with next:
>> ...
>>
>> These are all cases where we now miss things that should use
>> FREE_AND_NULL(), e.g.:
>>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index c973bd96dc..2f89338ed7 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -484,7 +484,8 @@ static int run_applypatch_msg_hook(struct am_state
>> *state)
>> ret = run_hook_le(NULL, "applypatch-msg", am_path(state,
>> "final-commit"), NULL);
>>
>> if (!ret) {
>> - FREE_AND_NULL(state->msg);
>> + free(state->msg);
>> + state->msg = NULL;
>> if (read_commit_msg(state) < 0)
>> die(_("'%s' was deleted by the applypatch-msg
>> hook"),
>> am_path(state, "final-commit"));
>>
>> So it looks to me like removing the "type T" rule breaks a lot of
>> things, but that the change you made to the expression rule is good, and
>> we should do that for the "type" rule as well. Your commit says the
>> "expression rule already covers it", but this doesn't seem to be the
>> case at all.
>
> OK, if an older version of the tool that is otherwise still usable
> needs two rules, let's keep the "type T" one by reverting the change
> out of 'next' and then have a patch that only rearranges the lines,
> something like the attached.
>
> -- >8 --
> From: René Scharfe
> Date: Sun, 25 Jun 2017 10:01:04 +0200
> Subject: [PATCH] coccinelle: polish FREE_AND_NULL rules
>
> There are two rules for using FREE_AND_NULL in free.cocci, one for
> pointer types and one for expressions. Both cause coccinelle to remove
> empty lines and even newline characters between replacements for some
> reason; consecutive "free(x);/x=NULL;" sequences end up as multiple
> FREE_AND_NULL calls on the same time.
>
> Rearrange the lines to place the addition of FREE_AND_NULL between
> the removals, which causes coccinelle to leave surrounding
> whitespace untouched.
>
> Signed-off-by: Rene Scharfe
> Signed-off-by: Junio C Hamano
> ---
> contrib/coccinelle/free.cocci | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
> index f2d97e755b..179c185d85 100644
> --- a/contrib/coccinelle/free.cocci
> +++ b/contrib/coccinelle/free.cocci
> @@ -15,12 +15,12 @@ type T;
> T *ptr;
> @@
> - free(ptr);
> -- ptr = NULL;
> + FREE_AND_NULL(ptr);
> +- ptr = NULL;
>
> @@
> expression E;
> @@
> - free(E);
> -- E = NULL;
> + FREE_AND_NULL(E);
> +- E = NULL;
Looks good.
What's cooking in git.git (Jun 2017, #08; Wed, 28)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * dt/raise-core-packed-git-limit (2017-06-21) 1 commit (merged to 'next' on 2017-06-22 at bc1a90f077) + docs: update 64-bit core.packedGitLimit default Doc update for a topic already in 'master'. * jk/add-p-commentchar-fix (2017-06-21) 2 commits (merged to 'next' on 2017-06-22 at 5a1d464e6d) + add--interactive: quote commentChar regex + add--interactive: handle EOF in prompt_yesno "git add -p" were updated in 2.12 timeframe to cope with custom core.commentchar but the implementation was buggy and a metacharacter like $ and * did not work. * ks/t7508-indent-fix (2017-06-21) 1 commit (merged to 'next' on 2017-06-22 at d12526967b) + t7508: fix a broken indentation Cosmetic update to a test. * lb/status-stash-count (2017-06-18) 3 commits (merged to 'next' on 2017-06-22 at 86bc2f2213) + glossary: define 'stash entry' + status: add optional stash count information + stash: update documentation to use 'stash entry' "git status" learned to optionally give how many stash entries the user has in its output. * mb/reword-autocomplete-message (2017-06-21) 1 commit (merged to 'next' on 2017-06-22 at 87a743) + auto-correct: tweak phrasing Message update. * mh/packed-ref-store-prep (2017-06-18) 2 commits (merged to 'next' on 2017-06-22 at 3f7a4da1e8) + for_each_bisect_ref(): don't trim refnames + lock_packed_refs(): fix cache validity check (this branch is used by mh/packed-ref-store and mh/packed-ref-store-prep-extra.) Bugfix for a topic that is (only) in 'master'. -- [New Topics] * ab/strbuf-addftime-tzname-boolify (2017-06-24) 3 commits - REWORD ONLY SQUASH - strbuf: change an always NULL/"" strbuf_addftime() param to bool - strbuf.h comment: discuss strbuf_addftime() arguments in order strbuf_addftime() is further getting tweaked. Waiting for a response. cf. * jc/pack-bitmap-unaligned (2017-06-26) 1 commit (merged to 'next' on 2017-06-28 at ad026b12a3) + pack-bitmap: don't perform unaligned memory access An unaligned 32-bit access in pack-bitmap code ahs been corrected. Will merge to 'master'. * mt/p4-parse-G-output (2017-06-27) 1 commit . git-p4: parse marshal output "p4 -G" in p4 changes Use "p4 -G" to make "p4 changes" output more Python-friendly to parse. Needs review/ack from git-p4 folks. * rs/apply-validate-input (2017-06-27) 3 commits (merged to 'next' on 2017-06-28 at 81e006b50e) + apply: check git diffs for mutually exclusive header lines + apply: check git diffs for invalid file modes + apply: check git diffs for missing old filenames Tighten error checks for invalid "git apply" input. Will merge to 'master'. * vs/typofixes (2017-06-27) 1 commit (merged to 'next' on 2017-06-28 at 3d11e0b3fa) + Spelling fixes Many typofixes. Will merge to 'master'. * aw/contrib-subtree-doc-asciidoctor (2017-06-27) 1 commit - subtree: honour USE_ASCIIDOCTOR when set The Makefile rule in contrib/subtree for building documentation learned to honour USE_ASCIIDOCTOR just like the main documentation set does. Will merge to 'next'. * js/fsck-name-object (2017-06-28) 1 commit - t1450: use egrep for regexp "alternation" Test fix. Will merge to 'next'. * jc/utf8-fprintf (2017-06-28) 1 commit - submodule--helper: do not call utf8_fprintf() unnecessarily Code cleanup. Will merge to 'next'. * rs/free-and-null (2017-06-28) 1 commit - coccinelle: polish FREE_AND_NULL rules Code cleanup. -- [Stalled] * mg/status-in-progress-info (2017-05-10) 2 commits - status --short --inprogress: spell it as --in-progress - status: show in-progress info for short status "git status" learns an option to report various operations (e.g. "merging") that the user is in the middle of. cf. * nd/worktree-move (2017-04-20) 6 commits - worktree remove: new command - worktree move: refuse to move worktrees with submodules - worktree move: accept destination as directory - worktree move: new command - worktree.c: add update_worktree_location() - worktree.c: add validate_worktree() "git worktree" learned move and remove subcommands. Expecting a reroll. cf. <20170420101024.7593-1-pclo...@gmail.com> cf. <20170421145916.mknekgqzhxffu...@sigill.intra.peff.net> cf. * xz/send-email-batch-size (2017-05-23) 1 commit - send-email: --batch-size to work around some SMTP server limit "git send-email" learned to overcome some SMTP se
Re: [PATCH v2 0/3] update sha1dc from PR #36
Ævar Arnfjörð Bjarmason writes: > On Tue, Jun 27 2017, Junio C. Hamano jotted: > >> Ævar Arnfjörð Bjarmason writes: >> >>> A v2 addressing comments to the v1, tbdiff below. Just fixes the >>> subject line on 1/3 & gets rid of the redundant _BIG_ENDIAN detection >>> for Solaris, we can just use __sparc (and indeed we did before this). >> >> Thanks. Let's have this tested by minority platform folks to help >> the upstream digest your pull request. The change since the last >> round makes perfect sense to me. > > Just a brief update. This has been found not to work on some BSDs (with > older GCC), but there's a patch being discussed at > https://github.com/cr-marcstevens/sha1collisiondetection/pull/34 > > The discussion is sorted out, just waiting on n-soda to submit a patch > (or to tell me he'd like me to), then the known issues will be fixed & > it should be merged quickly, then I'll submit a v3. Thanks. It's good that I learned to pretend that I didn't hear anything like I'm pretty sure this un-breaks everything & doesn't regress on cases where we fixed things earlier. from anybody, and instead wait a bit longer ;-)
Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
Ævar Arnfjörð Bjarmason writes:
> Late reply, sorry. What version of coccinelle are you running? I have
> 1.0.4 (from Debian) and can't get this to produce the same results as
> what I have.
>
> On top of next, I did:
>
> Revert "*.[ch] refactoring: make use of the FREE_AND_NULL() macro"
> Revert "coccinelle: make use of the "expression" FREE_AND_NULL() rule"
> Revert "coccinelle: make use of the "type" FREE_AND_NULL() rule"
>
> And then generated the patch as usual with `make coccicheck`, and
> applied it. This has your change.
>
> I then re-applied the manual "*.[ch] refactoring" changes
>
> This results in this diff with next:
> ...
>
> These are all cases where we now miss things that should use
> FREE_AND_NULL(), e.g.:
>
> diff --git a/builtin/am.c b/builtin/am.c
> index c973bd96dc..2f89338ed7 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -484,7 +484,8 @@ static int run_applypatch_msg_hook(struct am_state
> *state)
> ret = run_hook_le(NULL, "applypatch-msg", am_path(state,
> "final-commit"), NULL);
>
> if (!ret) {
> - FREE_AND_NULL(state->msg);
> + free(state->msg);
> + state->msg = NULL;
> if (read_commit_msg(state) < 0)
> die(_("'%s' was deleted by the applypatch-msg
> hook"),
> am_path(state, "final-commit"));
>
> So it looks to me like removing the "type T" rule breaks a lot of
> things, but that the change you made to the expression rule is good, and
> we should do that for the "type" rule as well. Your commit says the
> "expression rule already covers it", but this doesn't seem to be the
> case at all.
OK, if an older version of the tool that is otherwise still usable
needs two rules, let's keep the "type T" one by reverting the change
out of 'next' and then have a patch that only rearranges the lines,
something like the attached.
-- >8 --
From: René Scharfe
Date: Sun, 25 Jun 2017 10:01:04 +0200
Subject: [PATCH] coccinelle: polish FREE_AND_NULL rules
There are two rules for using FREE_AND_NULL in free.cocci, one for
pointer types and one for expressions. Both cause coccinelle to remove
empty lines and even newline characters between replacements for some
reason; consecutive "free(x);/x=NULL;" sequences end up as multiple
FREE_AND_NULL calls on the same time.
Rearrange the lines to place the addition of FREE_AND_NULL between
the removals, which causes coccinelle to leave surrounding
whitespace untouched.
Signed-off-by: Rene Scharfe
Signed-off-by: Junio C Hamano
---
contrib/coccinelle/free.cocci | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
index f2d97e755b..179c185d85 100644
--- a/contrib/coccinelle/free.cocci
+++ b/contrib/coccinelle/free.cocci
@@ -15,12 +15,12 @@ type T;
T *ptr;
@@
- free(ptr);
-- ptr = NULL;
+ FREE_AND_NULL(ptr);
+- ptr = NULL;
@@
expression E;
@@
- free(E);
-- E = NULL;
+ FREE_AND_NULL(E);
+- E = NULL;
--
2.13.2-659-g904a3dfd54
[PATCH 1/5] grep: remove redundant double assignment to 0
Stop assigning 0 to the extended_regexp_option field right after we've
zeroed out the entire struct with memset() just a few lines earlier.
Unlike some of the code being refactored in subsequent commits, this
was always completely redundant. See the original code introduced in
84befcd0a4 ("grep: add a grep.patternType configuration setting",
2012-08-03).
Signed-off-by: Ævar Arnfjörð Bjarmason
---
grep.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/grep.c b/grep.c
index 98733db623..29439886e7 100644
--- a/grep.c
+++ b/grep.c
@@ -38,7 +38,6 @@ void init_grep_defaults(void)
opt->regflags = REG_NEWLINE;
opt->max_depth = -1;
opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
- opt->extended_regexp_option = 0;
color_set(opt->color_context, "");
color_set(opt->color_filename, "");
color_set(opt->color_function, "");
--
2.13.1.611.g7e3b11ae1
[PATCH 4/5] grep: remove redundant and verbose re-assignments to 0
Remove the redundant re-assignments of the fixed/pcre1/pcre2 fields to
zero right after the entire struct has been set to zero via
memset(...).
See an earlier related cleanup commit e0b9f8ae09 ("grep: remove
redundant regflags assignments", 2017-05-25) for an explanation of why
the code was structured like this to begin with.
Signed-off-by: Ævar Arnfjörð Bjarmason
---
grep.c | 11 ---
1 file changed, 11 deletions(-)
diff --git a/grep.c b/grep.c
index 7cd8a6512f..736e1e00d6 100644
--- a/grep.c
+++ b/grep.c
@@ -175,28 +175,18 @@ static void grep_set_pattern_type_option(enum
grep_pattern_type pattern_type, st
/* fall through */
case GREP_PATTERN_TYPE_BRE:
- opt->fixed = 0;
- opt->pcre1 = 0;
- opt->pcre2 = 0;
break;
case GREP_PATTERN_TYPE_ERE:
- opt->fixed = 0;
- opt->pcre1 = 0;
- opt->pcre2 = 0;
opt->regflags |= REG_EXTENDED;
break;
case GREP_PATTERN_TYPE_FIXED:
opt->fixed = 1;
- opt->pcre1 = 0;
- opt->pcre2 = 0;
break;
case GREP_PATTERN_TYPE_PCRE:
- opt->fixed = 0;
#ifdef USE_LIBPCRE2
- opt->pcre1 = 0;
opt->pcre2 = 1;
#else
/*
@@ -206,7 +196,6 @@ static void grep_set_pattern_type_option(enum
grep_pattern_type pattern_type, st
* "cannot use Perl-compatible regexes[...]".
*/
opt->pcre1 = 1;
- opt->pcre2 = 0;
#endif
break;
}
--
2.13.1.611.g7e3b11ae1
[PATCH 5/5] grep: remove regflags from the public grep_opt API
Refactor calls to the grep machinery to always pass opt.ignore_case &
opt.extended_regexp_option instead of setting the equivalent regflags
bits.
The bug fixed when making -i work with -P in commit 9e3cbc59d5 ("log:
make --regexp-ignore-case work with --perl-regexp", 2017-05-20) was
really just plastering over the code smell which this change fixes.
See my "Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with
--perl-regexp"[1] for the discussion leading up to this.
The reason for adding the extensive commentary here is that I
discovered some subtle complexity in implementing this that really
should be called out explicitly to future readers.
Before this change we'd rely on the difference between
`extended_regexp_option` and `regflags` to serve as a membrane between
our preliminary parsing of grep.extendedRegexp and grep.patternType,
and what we decided to do internally.
Now that those two are the same thing, it's necessary to unset
`extended_regexp_option` just before we commit in cases where both of
those config variables are set. See 84befcd0a4 ("grep: add a
grep.patternType configuration setting", 2012-08-03) for the code and
documentation related to that.
The explanation of why the if/else branches in
grep_commit_pattern_type() are ordered the way they are exists in that
commit message, but I think it's worth calling this subtlety out
explicitly with a comment for future readers.
Unrelated to that: I could have factored out the default REG_NEWLINE
flag into some custom GIT_GREP_H_DEFAULT_REGFLAGS or something, but
since it's just used in two places I didn't think it was worth the
effort.
As an aside we're really lacking test coverage regflags being
initiated as 0 instead of as REG_NEWLINE. Tests will fail if it's
removed from compile_regexp(), but not if it's removed from
compile_fixed_regexp(). I have not dug to see if it's actually needed
in the latter case or if the test coverage is lacking.
1.
(https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1k...@mail.gmail.com/)
Signed-off-by: Ævar Arnfjörð Bjarmason
---
builtin/grep.c | 2 --
grep.c | 43 ++-
grep.h | 1 -
revision.c | 2 --
4 files changed, 34 insertions(+), 14 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index f61a9d938b..b682966439 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char
*prefix)
if (!opt.pattern_list)
die(_("no pattern given."));
- if (!opt.fixed && opt.ignore_case)
- opt.regflags |= REG_ICASE;
/*
* We have to find "--" in a separate pass, because its presence
diff --git a/grep.c b/grep.c
index 736e1e00d6..51aaad9f03 100644
--- a/grep.c
+++ b/grep.c
@@ -35,7 +35,6 @@ void init_grep_defaults(void)
memset(opt, 0, sizeof(*opt));
opt->relative = 1;
opt->pathname = 1;
- opt->regflags = REG_NEWLINE;
opt->max_depth = -1;
opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
color_set(opt->color_context, "");
@@ -154,7 +153,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)
opt->linenum = def->linenum;
opt->max_depth = def->max_depth;
opt->pathname = def->pathname;
- opt->regflags = def->regflags;
opt->relative = def->relative;
opt->output = def->output;
@@ -170,6 +168,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type,
struct grep_opt *opt)
{
+ /*
+* When committing to the pattern type by setting the relevant
+* fields in grep_opt it's generally not necessary to zero out
+* the fields we're not choosing, since they won't have been
+* set by anything. The extended_regexp_option field is the
+* only exception to this.
+*
+* This is because in the process of parsing grep.patternType
+* & grep.extendedRegexp we set opt->pattern_type_option and
+* opt->extended_regexp_option, respectively. We then
+* internally use opt->extended_regexp_option to see if we're
+* compiling an ERE. It must be unset if that's not actually
+* the case.
+*/
+ if (pattern_type != GREP_PATTERN_TYPE_ERE &&
+ opt->extended_regexp_option)
+ opt->extended_regexp_option = 0;
+
switch (pattern_type) {
case GREP_PATTERN_TYPE_UNSPECIFIED:
/* fall through */
@@ -178,7 +194,7 @@ static void grep_set_pattern_type_option(enum
grep_pattern_type pattern_type, st
break;
case GREP_PATTERN_TYPE_ERE:
- opt->regflags |= REG_EXTENDED;
+ opt->extended_regexp_option = 1;
break;
case GREP_PATTERN_TYPE_FIXED:
@@ -208,6 +224,11 @@ void grep_commit_pattern_type(enum grep_pa
[PATCH 3/5] grep: remove redundant "fixed" field re-assignment to 0
Remove the redundant re-assignment of the fixed field to zero right
after the entire struct has been set to zero via memset(...).
Unlike some nearby commits this pattern doesn't date back to the
pattern described in e0b9f8ae09 ("grep: remove redundant regflags
assignments", 2017-05-25), instead it was apparently cargo-culted in
9eceddeec6 ("Use kwset in grep", 2011-08-21).
Signed-off-by: Ævar Arnfjörð Bjarmason
---
grep.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/grep.c b/grep.c
index 6614042fdc..7cd8a6512f 100644
--- a/grep.c
+++ b/grep.c
@@ -627,8 +627,6 @@ static void compile_regexp(struct grep_pat *p, struct
grep_opt *opt)
has_null(p->pattern, p->patternlen) ||
is_fixed(p->pattern, p->patternlen))
p->fixed = !icase || ascii_only;
- else
- p->fixed = 0;
if (p->fixed) {
p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);
--
2.13.1.611.g7e3b11ae1
[PATCH 2/5] grep: remove redundant grep pattern type assignment
Remove a redundant assignment to extended_regexp_option to make it
zero if grep.extendedRegexp is not set. This is always called right
after init_grep_defaults() which memsets the entire structure to 0.
This is a logical follow-up to my commit to remove redundant regflags
assignments[1]. This logic was originally introduced in [2], but as
explained in the former commit it's working around a pattern in our
code that no longer exists, and is now confusing as it leads the
reader to think that this needs to be flipped back & forth.
1. e0b9f8ae09 ("grep: remove redundant regflags assignments",
2017-05-25)
2. b22520a37c ("grep: allow -E and -n to be turned on by default via
configuration", 2011-03-30)
Signed-off-by: Ævar Arnfjörð Bjarmason
---
grep.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/grep.c b/grep.c
index 29439886e7..6614042fdc 100644
--- a/grep.c
+++ b/grep.c
@@ -80,8 +80,6 @@ int grep_config(const char *var, const char *value, void *cb)
if (!strcmp(var, "grep.extendedregexp")) {
if (git_config_bool(var, value))
opt->extended_regexp_option = 1;
- else
- opt->extended_regexp_option = 0;
return 0;
}
--
2.13.1.611.g7e3b11ae1
[PATCH 0/5] grep: remove redundant code & reflags from API
Here's a follow-up to a small tangent of discussion in my ~30 patch grep cleanup series. There are no functional changes here, just getting rid of dead code, and removing the POSIX `regflags` variable from the grep API used by grep/log, which was the cause of the long-standing bug of "-i" not working with PCRE when used via git-log. Ævar Arnfjörð Bjarmason (5): grep: remove redundant double assignment to 0 grep: remove redundant grep pattern type assignment grep: remove redundant "fixed" field re-assignment to 0 grep: remove redundant and verbose re-assignments to 0 grep: remove regflags from the public grep_opt API builtin/grep.c | 2 -- grep.c | 59 +- grep.h | 1 - revision.c | 2 -- 4 files changed, 34 insertions(+), 30 deletions(-) -- 2.13.1.611.g7e3b11ae1
Re: [PATCH v2 0/3] update sha1dc from PR #36
On Tue, Jun 27 2017, Junio C. Hamano jotted: > Ævar Arnfjörð Bjarmason writes: > >> A v2 addressing comments to the v1, tbdiff below. Just fixes the >> subject line on 1/3 & gets rid of the redundant _BIG_ENDIAN detection >> for Solaris, we can just use __sparc (and indeed we did before this). > > Thanks. Let's have this tested by minority platform folks to help > the upstream digest your pull request. The change since the last > round makes perfect sense to me. Just a brief update. This has been found not to work on some BSDs (with older GCC), but there's a patch being discussed at https://github.com/cr-marcstevens/sha1collisiondetection/pull/34 The discussion is sorted out, just waiting on n-soda to submit a patch (or to tell me he'd like me to), then the known issues will be fixed & it should be merged quickly, then I'll submit a v3.
Re: [PATCH v4 4/6] coccinelle: add a rule to make "expression" code use FREE_AND_NULL()
On Sun, Jun 25 2017, René Scharfe jotted:
> Am 16.06.2017 um 21:43 schrieb Junio C Hamano:
>> Ævar Arnfjörð Bjarmason writes:
>>
>>> A follow-up to the existing "type" rule added in an earlier
>>> change. This catches some occurrences that are missed by the previous
>>> rule.
>>>
>>> Signed-off-by: Ævar Arnfjörð Bjarmason
>>> ---
>>
>> Hmph, I wonder if the "type" thing is really needed. Over there,
>> "ptr" is an expression and we can find "free(ptr); ptr = NULL" with
>> the rule in this patch already, no?
>
> Indeed. How about this on top of master?
>
> -- >8 --
> Subject: [PATCH] coccinelle: polish FREE_AND_NULL rules
>
> There are two rules for using FREE_AND_NULL in free.cocci, one for
> pointer types and one for expressions. Both cause coccinelle to remove
> empty lines and even newline characters between replacements for some
> reason; consecutive "free(x);/x=NULL;" sequences end up as multiple
> FREE_AND_NULL calls on the same time.
>
> Remove the type rule, as the expression rule already covers it, and
> rearrange the lines of the latter to place the addition of FREE_AND_NULL
> between the removals, which causes coccinelle to leave surrounding
> whitespace untouched.
>
> Signed-off-by: Rene Scharfe
> ---
> contrib/coccinelle/free.cocci | 10 +-
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/contrib/coccinelle/free.cocci b/contrib/coccinelle/free.cocci
> index f2d97e755b..4490069df9 100644
> --- a/contrib/coccinelle/free.cocci
> +++ b/contrib/coccinelle/free.cocci
> @@ -11,16 +11,8 @@ expression E;
>free(E);
>
> @@
> -type T;
> -T *ptr;
> -@@
> -- free(ptr);
> -- ptr = NULL;
> -+ FREE_AND_NULL(ptr);
> -
> -@@
> expression E;
> @@
> - free(E);
> -- E = NULL;
> + FREE_AND_NULL(E);
> +- E = NULL;
Late reply, sorry. What version of coccinelle are you running? I have
1.0.4 (from Debian) and can't get this to produce the same results as
what I have.
On top of next, I did:
Revert "*.[ch] refactoring: make use of the FREE_AND_NULL() macro"
Revert "coccinelle: make use of the "expression" FREE_AND_NULL() rule"
Revert "coccinelle: make use of the "type" FREE_AND_NULL() rule"
And then generated the patch as usual with `make coccicheck`, and
applied it. This has your change.
I then re-applied the manual "*.[ch] refactoring" changes
This results in this diff with next:
$ git diff --stat origin/next.. -- '*.[ch]'
builtin/am.c | 3 ++-
builtin/clean.c | 6 --
builtin/config.c | 6 --
builtin/index-pack.c | 6 --
builtin/pack-objects.c | 12
builtin/unpack-objects.c | 3 ++-
fast-import.c| 6 --
http-push.c | 24
http.c | 15 ++-
imap-send.c | 3 ++-
ref-filter.c | 3 ++-
refs/files-backend.c | 3 ++-
remote-testsvn.c | 3 ++-
sequencer.c | 3 ++-
sha1-array.c | 3 ++-
sha1_file.c | 3 ++-
transport-helper.c | 27 ++-
transport.c | 3 ++-
tree-diff.c | 6 --
tree.c | 3 ++-
20 files changed, 94 insertions(+), 47 deletions(-)
These are all cases where we now miss things that should use
FREE_AND_NULL(), e.g.:
diff --git a/builtin/am.c b/builtin/am.c
index c973bd96dc..2f89338ed7 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -484,7 +484,8 @@ static int run_applypatch_msg_hook(struct am_state
*state)
ret = run_hook_le(NULL, "applypatch-msg", am_path(state,
"final-commit"), NULL);
if (!ret) {
- FREE_AND_NULL(state->msg);
+ free(state->msg);
+ state->msg = NULL;
if (read_commit_msg(state) < 0)
die(_("'%s' was deleted by the applypatch-msg
hook"),
am_path(state, "final-commit"));
So it looks to me like removing the "type T" rule breaks a lot of
things, but that the change you made to the expression rule is good, and
we should do that for the "type" rule as well. Your commit says the
"expression rule already covers it", but this doesn't seem to be the
case at all.
As an aside: Junio, did you mean to apply f8bb4631fb to next this way?
Looks like a mis-applied scissor commit.
[PATCH v8 5/6] convert: move multiple file filter error handling to separate function
Refactoring the filter error handling is useful for the subsequent patch
'convert: add "status=delayed" to filter process protocol'.
In addition, replace the parentheses around the empty "if" block with a
single semicolon to adhere to the Git style guide.
Signed-off-by: Lars Schneider
---
convert.c | 47 ++-
1 file changed, 26 insertions(+), 21 deletions(-)
diff --git a/convert.c b/convert.c
index 9907e3b9ba..e55c034d86 100644
--- a/convert.c
+++ b/convert.c
@@ -565,6 +565,29 @@ static int start_multi_file_filter_fn(struct
subprocess_entry *subprocess)
return err;
}
+static void handle_filter_error(const struct strbuf *filter_status,
+ struct cmd2process *entry,
+ const unsigned int wanted_capability) {
+ if (!strcmp(filter_status->buf, "error"))
+ ; /* The filter signaled a problem with the file. */
+ else if (!strcmp(filter_status->buf, "abort") && wanted_capability) {
+ /*
+* The filter signaled a permanent problem. Don't try to filter
+* files with the same command for the lifetime of the current
+* Git process.
+*/
+entry->supported_capabilities &= ~wanted_capability;
+ } else {
+ /*
+* Something went wrong with the protocol filter.
+* Force shutdown and restart if another blob requires
filtering.
+*/
+ error("external filter '%s' failed", entry->subprocess.cmd);
+ subprocess_stop(&subprocess_map, &entry->subprocess);
+ free(entry);
+ }
+}
+
static int apply_multi_file_filter(const char *path, const char *src, size_t
len,
int fd, struct strbuf *dst, const char *cmd,
const unsigned int wanted_capability)
@@ -656,28 +679,10 @@ static int apply_multi_file_filter(const char *path,
const char *src, size_t len
done:
sigchain_pop(SIGPIPE);
- if (err) {
- if (!strcmp(filter_status.buf, "error")) {
- /* The filter signaled a problem with the file. */
- } else if (!strcmp(filter_status.buf, "abort")) {
- /*
-* The filter signaled a permanent problem. Don't try
to filter
-* files with the same command for the lifetime of the
current
-* Git process.
-*/
-entry->supported_capabilities &= ~wanted_capability;
- } else {
- /*
-* Something went wrong with the protocol filter.
-* Force shutdown and restart if another blob requires
filtering.
-*/
- error("external filter '%s' failed", cmd);
- subprocess_stop(&subprocess_map, &entry->subprocess);
- free(entry);
- }
- } else {
+ if (err)
+ handle_filter_error(&filter_status, entry, wanted_capability);
+ else
strbuf_swap(dst, &nbuf);
- }
strbuf_release(&nbuf);
return !err;
}
--
2.13.2
[PATCH v8 6/6] convert: add "status=delayed" to filter process protocol
Some `clean` / `smudge` filters may require a significant amount of
time to process a single blob (e.g. the Git LFS smudge filter might
perform network requests). During this process the Git checkout
operation is blocked and Git needs to wait until the filter is done to
continue with the checkout.
Teach the filter process protocol, introduced in edcc8581 ("convert: add
filter..process option", 2016-10-16), to accept the status
"delayed" as response to a filter request. Upon this response Git
continues with the checkout operation. After the checkout operation Git
calls "finish_delayed_checkout" which queries the filter for remaining
blobs. If the filter is still working on the completion, then the filter
is expected to block. If the filter has completed all remaining blobs
then an empty response is expected.
Git has a multiple code paths that checkout a blob. Support delayed
checkouts only in `clone` (in unpack-trees.c) and `checkout` operations
for now. The optimization is most effective in these code paths as all
files of the tree are processed.
Signed-off-by: Lars Schneider
---
Documentation/gitattributes.txt | 69 +-
builtin/checkout.c | 3 +
cache.h | 3 +
convert.c | 149 +++--
convert.h | 26 +
entry.c | 132 +-
t/t0021-conversion.sh | 116 +++
t/t0021/rot13-filter.pl | 204 +++-
unpack-trees.c | 2 +
9 files changed, 602 insertions(+), 102 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 4736483865..4049a0b9a8 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -425,8 +425,8 @@ packet: git< capability=clean
packet: git< capability=smudge
packet: git<
-Supported filter capabilities in version 2 are "clean" and
-"smudge".
+Supported filter capabilities in version 2 are "clean", "smudge",
+and "delay".
Afterwards Git sends a list of "key=value" pairs terminated with
a flush packet. The list will contain at least the filter command
@@ -512,12 +512,73 @@ the protocol then Git will stop the filter process and
restart it
with the next file that needs to be processed. Depending on the
`filter..required` flag Git will interpret that as error.
-After the filter has processed a blob it is expected to wait for
-the next "key=value" list containing a command. Git will close
+After the filter has processed a command it is expected to wait for
+a "key=value" list containing the next command. Git will close
the command pipe on exit. The filter is expected to detect EOF
and exit gracefully on its own. Git will wait until the filter
process has stopped.
+Delay
+^
+
+If the filter supports the "delay" capability, then Git can send the
+flag "can-delay" after the filter command and pathname. This flag
+denotes that the filter can delay filtering the current blob (e.g. to
+compensate network latencies) by responding with no content but with
+the status "delayed" and a flush packet.
+
+packet: git> command=smudge
+packet: git> pathname=path/testfile.dat
+packet: git> can-delay=1
+packet: git>
+packet: git> CONTENT
+packet: git>
+packet: git< status=delayed
+packet: git<
+
+
+If the filter supports the "delay" capability then it must support the
+"list_available_blobs" command. If Git sends this command, then the
+filter is expected to return a list of pathnames representing blobs
+that have been delayed earlier and are now available.
+The list must be terminated with a flush packet followed
+by a "success" status that is also terminated with a flush packet. If
+no blobs for the delayed paths are available, yet, then the filter is
+expected to block the response until at least one blob becomes
+available. The filter can tell Git that it has no more delayed blobs
+by sending an empty list. As soon as the filter responds with an empty
+list, Git stops asking. All blobs that Git has not received at this
+point are considered missing and will result in an error.
+
+
+packet: git> command=list_available_blobs
+packet: git>
+packet: git< pathname=path/testfile.dat
+packet: git< pathname=path/otherfile.dat
+packet: git<
+packet: git< status=success
+packet: git<
+
+
+After Git received the pathnames, it will request the corresponding
+blobs again. These requests contain a pathname and an empty content
+section. The filter is expected to respond with the smudged content
+in the usual way as explained above.
+
+packet:
[PATCH v8 3/6] t0021: write "OUT " only on success
"rot13-filter.pl" always writes "OUT " to the debug log at the end
of a response.
This works perfectly for the existing responses "abort", "error", and
"success". A new response "delayed", that will be introduced in a
subsequent patch, accepts the input without giving the filtered result
right away. At this point we cannot know the size of the response.
Therefore, we do not write "OUT " for "delayed" responses.
To simplify the code we do not write "OUT " for "abort" and
"error" responses either as their size is always zero.
Signed-off-by: Lars Schneider
---
t/t0021-conversion.sh | 6 +++---
t/t0021/rot13-filter.pl | 6 +++---
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 0139b460e7..0c04d346a1 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -588,7 +588,7 @@ test_expect_success PERL 'process filter should restart
after unexpected write f
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge smudge-write-fail.r $SF [OK] -- OUT: $SF
[WRITE FAIL]
+ IN: smudge smudge-write-fail.r $SF [OK] -- [WRITE FAIL]
START
init handshake complete
IN: smudge test.r $S [OK] -- OUT: $S . [OK]
@@ -634,7 +634,7 @@ test_expect_success PERL 'process filter should not be
restarted if it signals a
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge error.r $SE [OK] -- OUT: 0 [ERROR]
+ IN: smudge error.r $SE [OK] -- [ERROR]
IN: smudge test.r $S [OK] -- OUT: $S . [OK]
IN: smudge test2.r $S2 [OK] -- OUT: $S2 . [OK]
STOP
@@ -673,7 +673,7 @@ test_expect_success PERL 'process filter abort stops
processing of all further f
cat >expected.log <<-EOF &&
START
init handshake complete
- IN: smudge abort.r $SA [OK] -- OUT: 0 [ABORT]
+ IN: smudge abort.r $SA [OK] -- [ABORT]
STOP
EOF
test_cmp_exclude_clean expected.log debug.log &&
diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index 0b943bb377..5e43faeec1 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -153,9 +153,6 @@ while (1) {
die "bad command '$command'";
}
- print $debug "OUT: " . length($output) . " ";
- $debug->flush();
-
if ( $pathname eq "error.r" ) {
print $debug "[ERROR]\n";
$debug->flush();
@@ -178,6 +175,9 @@ while (1) {
die "${command} write error";
}
+ print $debug "OUT: " . length($output) . " ";
+ $debug->flush();
+
while ( length($output) > 0 ) {
my $packet = substr( $output, 0,
$MAX_PACKET_CONTENT_SIZE );
packet_bin_write($packet);
--
2.13.2
[PATCH v8 2/6] t0021: make debug log file name configurable
The "rot13-filter.pl" helper wrote its debug logs always to "rot13-filter.log".
Make this configurable by defining the log file as first parameter of
"rot13-filter.pl".
This is useful if "rot13-filter.pl" is configured multiple times similar to the
subsequent patch 'convert: add "status=delayed" to filter process protocol'.
Signed-off-by: Lars Schneider
---
t/t0021-conversion.sh | 44 ++--
t/t0021/rot13-filter.pl | 8 +---
2 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index ff2424225b..0139b460e7 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -28,7 +28,7 @@ file_size () {
}
filter_git () {
- rm -f rot13-filter.log &&
+ rm -f *.log &&
git "$@"
}
@@ -342,7 +342,7 @@ test_expect_success 'diff does not reuse worktree files
that need cleaning' '
'
test_expect_success PERL 'required process filter should filter data' '
- test_config_global filter.protocol.process "rot13-filter.pl clean
smudge" &&
+ test_config_global filter.protocol.process "rot13-filter.pl debug.log
clean smudge" &&
test_config_global filter.protocol.required true &&
rm -rf repo &&
mkdir repo &&
@@ -375,7 +375,7 @@ test_expect_success PERL 'required process filter should
filter data' '
IN: clean testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK]
-- OUT: $S3 . [OK]
STOP
EOF
- test_cmp_count expected.log rot13-filter.log &&
+ test_cmp_count expected.log debug.log &&
git commit -m "test commit 2" &&
rm -f test2.r "testsubdir/test3 '\''sq'\'',\$x=.r" &&
@@ -388,7 +388,7 @@ test_expect_success PERL 'required process filter should
filter data' '
IN: smudge testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK]
-- OUT: $S3 . [OK]
STOP
EOF
- test_cmp_exclude_clean expected.log rot13-filter.log &&
+ test_cmp_exclude_clean expected.log debug.log &&
filter_git checkout --quiet --no-progress empty-branch &&
cat >expected.log <<-EOF &&
@@ -397,7 +397,7 @@ test_expect_success PERL 'required process filter should
filter data' '
IN: clean test.r $S [OK] -- OUT: $S . [OK]
STOP
EOF
- test_cmp_exclude_clean expected.log rot13-filter.log &&
+ test_cmp_exclude_clean expected.log debug.log &&
filter_git checkout --quiet --no-progress master &&
cat >expected.log <<-EOF &&
@@ -409,7 +409,7 @@ test_expect_success PERL 'required process filter should
filter data' '
IN: smudge testsubdir/test3 '\''sq'\'',\$x=.r $S3 [OK]
-- OUT: $S3 . [OK]
STOP
EOF
- test_cmp_exclude_clean expected.log rot13-filter.log &&
+ test_cmp_exclude_clean expected.log debug.log &&
test_cmp_committed_rot13 "$TEST_ROOT/test.o" test.r &&
test_cmp_committed_rot13 "$TEST_ROOT/test2.o" test2.r &&
@@ -419,7 +419,7 @@ test_expect_success PERL 'required process filter should
filter data' '
test_expect_success PERL 'required process filter takes precedence' '
test_config_global filter.protocol.clean false &&
- test_config_global filter.protocol.process "rot13-filter.pl clean" &&
+ test_config_global filter.protocol.process "rot13-filter.pl debug.log
clean" &&
test_config_global filter.protocol.required true &&
rm -rf repo &&
mkdir repo &&
@@ -439,12 +439,12 @@ test_expect_success PERL 'required process filter takes
precedence' '
IN: clean test.r $S [OK] -- OUT: $S . [OK]
STOP
EOF
- test_cmp_count expected.log rot13-filter.log
+ test_cmp_count expected.log debug.log
)
'
test_expect_success PERL 'required process filter should be used only for
"clean" operation only' '
- test_config_global filter.protocol.process "rot13-filter.pl clean" &&
+ test_config_global filter.protocol.process "rot13-filter.pl debug.log
clean" &&
rm -rf repo &&
mkdir repo &&
(
@@ -462,7 +462,7 @@ test_expect_success PERL 'required process filter should be
used only for "clean
IN: clean test.r $S [OK] -- OUT: $S . [OK]
STOP
EOF
- test_cmp_count expected.log rot13-filter.log &&
+ test_cmp_count expected.log debug.log &&
rm test.r &&
@@ -474,12 +474,12 @@ test_expect_success PERL 'required process filter should
be used only for "clean
init handshake complete
STOP
EOF
- test_cmp
[PATCH v8 4/6] convert: put the flags field before the flag itself for consistent style
Suggested-by: Jeff King
Signed-off-by: Lars Schneider
---
convert.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/convert.c b/convert.c
index f1e168bc30..9907e3b9ba 100644
--- a/convert.c
+++ b/convert.c
@@ -597,12 +597,12 @@ static int apply_multi_file_filter(const char *path,
const char *src, size_t len
}
process = &entry->subprocess.process;
- if (!(wanted_capability & entry->supported_capabilities))
+ if (!(entry->supported_capabilities & wanted_capability))
return 0;
- if (CAP_CLEAN & wanted_capability)
+ if (wanted_capability & CAP_CLEAN)
filter_type = "clean";
- else if (CAP_SMUDGE & wanted_capability)
+ else if (wanted_capability & CAP_SMUDGE)
filter_type = "smudge";
else
die("unexpected filter type");
@@ -703,9 +703,9 @@ static int apply_filter(const char *path, const char *src,
size_t len,
if (!dst)
return 1;
- if ((CAP_CLEAN & wanted_capability) && !drv->process && drv->clean)
+ if ((wanted_capability & CAP_CLEAN) && !drv->process && drv->clean)
cmd = drv->clean;
- else if ((CAP_SMUDGE & wanted_capability) && !drv->process &&
drv->smudge)
+ else if ((wanted_capability & CAP_SMUDGE) && !drv->process &&
drv->smudge)
cmd = drv->smudge;
if (cmd && *cmd)
--
2.13.2
[PATCH v8 0/6] convert: add "status=delayed" to filter process protocol
Hi,
here is the 8th iteration of my "status delayed" topic. Patch 1 to 3 are
minor t0021 test adjustments. Patch 4 is a minor convert.c style adjustment.
Patch 5 is a minor "extract method" refactoring in convert.c with an
additional minor style adjustment. Patch 6 is the new feature.
### Changes since v7 (full inter diff below!):
* remove 'is_delayed' member from delayed_checkout struct as the
information can be derived from the 'paths' member (Junio)
* use table-driven approach for capability negotiation (Junio)
* remove unnecessary assert (Junio)
* fix comment style (Junio)
If you review this series then please read the "Delay" section in
"Documentation/gitattributes.txt" first for an overview of the delay mechanism.
The changes in "t/t0021/rot13-filter.pl" are easier to review if you ignore
whitespace changes.
Thanks,
Lars
RFC: http://public-inbox.org/git/d10f7c47-14e8-465b-8b7a-a09a1b28a...@gmail.com/
v1: http://public-inbox.org/git/20170108191736.47359-1-larsxschnei...@gmail.com/
v2: http://public-inbox.org/git/20170226184816.30010-1-larsxschnei...@gmail.com/
v3: http://public-inbox.org/git/20170409191107.20547-1-larsxschnei...@gmail.com/
v4: http://public-inbox.org/git/20170522135001.54506-1-larsxschnei...@gmail.com/
v5: http://public-inbox.org/git/20170601082203.50397-1-larsxschnei...@gmail.com/
v6: http://public-inbox.org/git/20170625182125.6741-1-larsxschnei...@gmail.com/
v7: http://public-inbox.org/git/20170627121027.99209-1-larsxschnei...@gmail.com/
Base Ref: master
Web-Diff: https://github.com/larsxschneider/git/commit/9877046063
Checkout: git fetch https://github.com/larsxschneider/git
filter-process/delay-v8 && git checkout 9877046063
### Interdiff (v7..v8):
diff --git a/convert.c b/convert.c
index f17d822ac8..12a0b3eafb 100644
--- a/convert.c
+++ b/convert.c
@@ -508,7 +508,7 @@ static struct hashmap subprocess_map;
static int start_multi_file_filter_fn(struct subprocess_entry *subprocess)
{
- int err;
+ int err, i;
struct cmd2process *entry = (struct cmd2process *)subprocess;
struct string_list cap_list = STRING_LIST_INIT_NODUP;
char *cap_buf;
@@ -516,6 +516,15 @@ static int start_multi_file_filter_fn(struct
subprocess_entry *subprocess)
struct child_process *process = &subprocess->process;
const char *cmd = subprocess->cmd;
+ static const struct {
+ const char *name;
+ unsigned int cap;
+ } known_caps[] = {
+ { "clean", CAP_CLEAN },
+ { "smudge", CAP_SMUDGE },
+ { "delay", CAP_DELAY },
+ };
+
sigchain_push(SIGPIPE, SIG_IGN);
err = packet_writel(process->in, "git-filter-client", "version=2",
NULL);
@@ -534,8 +543,15 @@ static int start_multi_file_filter_fn(struct
subprocess_entry *subprocess)
if (err)
goto done;
- err = packet_writel(process->in,
- "capability=clean", "capability=smudge", "capability=delay",
NULL);
+ for (i = 0; i < ARRAY_SIZE(known_caps); ++i) {
+ err = packet_write_fmt_gently(
+ process->in, "capability=%s\n", known_caps[i].name);
+ if (err)
+ goto done;
+ }
+ err = packet_flush_gently(process->in);
+ if (err)
+ goto done;
for (;;) {
cap_buf = packet_read_line(process->out, NULL);
@@ -547,18 +563,15 @@ static int start_multi_file_filter_fn(struct
subprocess_entry *subprocess)
continue;
cap_name = cap_list.items[1].string;
- if (!strcmp(cap_name, "clean")) {
- entry->supported_capabilities |= CAP_CLEAN;
- } else if (!strcmp(cap_name, "smudge")) {
- entry->supported_capabilities |= CAP_SMUDGE;
- } else if (!strcmp(cap_name, "delay")) {
- entry->supported_capabilities |= CAP_DELAY;
- } else {
- warning(
- "external filter '%s' requested unsupported
filter capability '%s'",
- cmd, cap_name
- );
- }
+ i = ARRAY_SIZE(known_caps) - 1;
+ while (i >= 0 && strcmp(cap_name, known_caps[i].name))
+ i--;
+
+ if (i >= 0)
+ entry->supported_capabilities |= known_caps[i].cap;
+ else
+ warning("external filter '%s' requested unsupported
filter capability '%s'",
+ cmd, cap_name);
string_list_clear(&cap_list, 0);
}
@@ -677,7 +690,6 @@ static int apply_multi_file_filter(const char *path, const
char *src, size_t len
goto done;
if (can_delay && !strcmp(filter_status.buf, "delayed")) {
- dco->is_delayed = 1;
string_list_insert(&dco->filters,
[PATCH v8 1/6] t0021: keep filter log files on comparison
The filter log files are modified on comparison. That might be
unexpected by the caller. It would be even undesirable if the caller
wants to reuse the original log files.
Address these issues by using temp files for modifications. This is
useful for the subsequent patch 'convert: add "status=delayed" to
filter process protocol'.
Signed-off-by: Lars Schneider
---
t/t0021-conversion.sh | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 161f560446..ff2424225b 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -42,10 +42,10 @@ test_cmp_count () {
for FILE in "$expect" "$actual"
do
sort "$FILE" | uniq -c |
- sed -e "s/^ *[0-9][0-9]*[ ]*IN: /x IN: /" >"$FILE.tmp" &&
- mv "$FILE.tmp" "$FILE" || return
+ sed -e "s/^ *[0-9][0-9]*[ ]*IN: /x IN: /" >"$FILE.tmp"
done &&
- test_cmp "$expect" "$actual"
+ test_cmp "$expect.tmp" "$actual.tmp" &&
+ rm "$expect.tmp" "$actual.tmp"
}
# Compare two files but exclude all `clean` invocations because Git can
@@ -56,10 +56,10 @@ test_cmp_exclude_clean () {
actual=$2
for FILE in "$expect" "$actual"
do
- grep -v "IN: clean" "$FILE" >"$FILE.tmp" &&
- mv "$FILE.tmp" "$FILE"
+ grep -v "IN: clean" "$FILE" >"$FILE.tmp"
done &&
- test_cmp "$expect" "$actual"
+ test_cmp "$expect.tmp" "$actual.tmp" &&
+ rm "$expect.tmp" "$actual.tmp"
}
# Check that the contents of two files are equal and that their rot13 version
--
2.13.2
Re: [PATCH] submodule--helper: do not call utf8_fprintf() unnecessarily
On Wed, Jun 28, 2017 at 1:58 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> On Wed, Jun 28, 2017 at 1:38 PM, Junio C Hamano wrote:
>>> The helper function utf8_fprintf(fp, ...) has exactly the same
>>> effect to the output stream fp as fprintf(fp, ...) does, and the
>>> only difference is that its return value counts in display columns
>>> consumed (assuming that the payload is encoded in UTF-8), as opposed
>>> to number of bytes.
>>>
>>> There is no reason to call it unless the caller cares about its
>>> return value.
>>>
>>> Signed-off-by: Junio C Hamano
>>> ---
>>>
>>> * The helper was introduced at c0821965 ("Add utf8_fprintf helper
>>>that returns correct number of columns", 2013-02-09), which also
>>>taught the help text output from the parse_options API to use it
>>>to align columns. These original callers naturally do use the
>>>returned value and left alone by this fix, which corrects all the
>>>later callers that misuses it.
>>>
>>
>> The patch looks correct.
I said this because I had a similar implementation a couple weeks back
when Peff tried to poke (security) holes into submodule usage.
I tried finding the reason why it was originally introduced, but to no avail.
It seems to be randomly introduced.
> Thanks. I had a small voice back in my head telling me that I may
> have misread the code and this patch breaks things, which you
> cleared up for me ;-)
That said, #include "utf8.h" could also go from the file with this
or after this patch, I believe.
Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes
Stefan Beller writes: > Originally I wanted to do that, see prep work in [1], but Jeff explained that > the additional pointer in the compare function is **not** supposed to be > a additional payload (such as the diff options specifying the white space > options.) > > [1] https://public-inbox.org/git/20170512200244.25245-1-sbel...@google.com/ Ah, yes, keydata is a wrong thing to use to give additional hint to the eqv function. We do need to fix the function signature of hashmap_cmp_fn. It is common for us to want to use a single implementation of an eqv function that can be configured to behave slightly differently but the customization will stay the same throughout the lifetime of a hashmap. IOW, hashmap_init() needs to be able to take a pointer to such a configuration data (e.g. diff_options), and then the comparison made between two elements that hash to the same bucket needs to be given not just the two elements but also that configuration data to tweak the function.
Re: [PATCH] submodule--helper: do not call utf8_fprintf() unnecessarily
Stefan Beller writes:
> On Wed, Jun 28, 2017 at 1:38 PM, Junio C Hamano wrote:
>> The helper function utf8_fprintf(fp, ...) has exactly the same
>> effect to the output stream fp as fprintf(fp, ...) does, and the
>> only difference is that its return value counts in display columns
>> consumed (assuming that the payload is encoded in UTF-8), as opposed
>> to number of bytes.
>>
>> There is no reason to call it unless the caller cares about its
>> return value.
>>
>> Signed-off-by: Junio C Hamano
>> ---
>>
>> * The helper was introduced at c0821965 ("Add utf8_fprintf helper
>>that returns correct number of columns", 2013-02-09), which also
>>taught the help text output from the parse_options API to use it
>>to align columns. These original callers naturally do use the
>>returned value and left alone by this fix, which corrects all the
>>later callers that misuses it.
>>
>
> The patch looks correct.
Thanks. I had a small voice back in my head telling me that I may
have misread the code and this patch breaks things, which you
cleared up for me ;-)
Re: [PATCH] submodule--helper: do not call utf8_fprintf() unnecessarily
On Wed, Jun 28, 2017 at 1:38 PM, Junio C Hamano wrote:
> The helper function utf8_fprintf(fp, ...) has exactly the same
> effect to the output stream fp as fprintf(fp, ...) does, and the
> only difference is that its return value counts in display columns
> consumed (assuming that the payload is encoded in UTF-8), as opposed
> to number of bytes.
>
> There is no reason to call it unless the caller cares about its
> return value.
>
> Signed-off-by: Junio C Hamano
> ---
>
> * The helper was introduced at c0821965 ("Add utf8_fprintf helper
>that returns correct number of columns", 2013-02-09), which also
>taught the help text output from the parse_options API to use it
>to align columns. These original callers naturally do use the
>returned value and left alone by this fix, which corrects all the
>later callers that misuses it.
>
The patch looks correct.
Thanks,
Stefan
> builtin/submodule--helper.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 8517032b3e..50c6af1de7 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -326,7 +326,7 @@ static int module_list(int argc, const char **argv, const
> char *prefix)
> printf("%06o %s %d\t", ce->ce_mode,
>oid_to_hex(&ce->oid), ce_stage(ce));
>
> - utf8_fprintf(stdout, "%s\n", ce->name);
> + fprintf(stdout, "%s\n", ce->name);
> }
> return 0;
> }
> @@ -1038,7 +1038,7 @@ static int update_clone(int argc, const char **argv,
> const char *prefix)
> return 1;
>
> for_each_string_list_item(item, &suc.projectlines)
> - utf8_fprintf(stdout, "%s", item->string);
> + fprintf(stdout, "%s", item->string);
>
> return 0;
> }
[PATCH] submodule--helper: do not call utf8_fprintf() unnecessarily
The helper function utf8_fprintf(fp, ...) has exactly the same
effect to the output stream fp as fprintf(fp, ...) does, and the
only difference is that its return value counts in display columns
consumed (assuming that the payload is encoded in UTF-8), as opposed
to number of bytes.
There is no reason to call it unless the caller cares about its
return value.
Signed-off-by: Junio C Hamano
---
* The helper was introduced at c0821965 ("Add utf8_fprintf helper
that returns correct number of columns", 2013-02-09), which also
taught the help text output from the parse_options API to use it
to align columns. These original callers naturally do use the
returned value and left alone by this fix, which corrects all the
later callers that misuses it.
builtin/submodule--helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 8517032b3e..50c6af1de7 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -326,7 +326,7 @@ static int module_list(int argc, const char **argv, const
char *prefix)
printf("%06o %s %d\t", ce->ce_mode,
oid_to_hex(&ce->oid), ce_stage(ce));
- utf8_fprintf(stdout, "%s\n", ce->name);
+ fprintf(stdout, "%s\n", ce->name);
}
return 0;
}
@@ -1038,7 +1038,7 @@ static int update_clone(int argc, const char **argv,
const char *prefix)
return 1;
for_each_string_list_item(item, &suc.projectlines)
- utf8_fprintf(stdout, "%s", item->string);
+ fprintf(stdout, "%s", item->string);
return 0;
}
Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes
On Tue, Jun 27, 2017 at 10:06 PM, Junio C Hamano wrote:
> Junio C Hamano writes:
>
>> Looking at the implementation of get_ws_cleaned_string() that is the
>> workhorse of emitted_symbol_cmp_no_ws(), it seems to be doing wrong
>> things for various "ignore whitespace" options (i.e. there is only
>> one implementation, while "git diff" family takes things like
>> "ignore space change", "ignore all whitespace", etc.), though.
>
> This probably deserves a bit more illustration of how I envision the
> code should evolve.
>
> In the longer term, I would prefer to see emitted_symbol_cmp_no_ws()
> to go and instead emitted_symbol_cmp() to take the diff options
> so
> that it can change the behaviour of the comparison function based on
> the -w/-b/--ignore-space-at-eol/etc. settings. And compare two strings
> in place.
ok, in-place is no problem. But passing down the diff options into the
compare function is a bit hard.
Originally I wanted to do that, see prep work in [1], but Jeff explained that
the additional pointer in the compare function is **not** supposed to be
a additional payload (such as the diff options specifying the white space
options.)
[1] https://public-inbox.org/git/20170512200244.25245-1-sbel...@google.com/
However as we no settled on the struct emitted_diff_symbol,
that has a 'flags' field in there, which ought to contain everything we
know about whitespace settings, we should be able to do that from there.
> emitted_symbol_eqv(struct emitted_diff_symbol *a,
>struct emitted_diff_symbol *b,
>const void *keydata) {
> struct diff_options *diffopt = keydata;
The prep work mentioned, would allow for this, as keydata
would be passed through as-is in all calls of the hashmap API,
such that the user can decide if they use it as the actual 'keydata'
or rather as an additional payload.
Thanks for outlining the idea in code, but maybe we need to
reconsider the hashmap API before that.
By not considering the change in the hashmap API, the current
implementation tried to get away by having different compare functions.
Thanks,
Stefan
Re: [PATCH 5/6] diff.c: omit uninteresting moved lines
On Tue, Jun 27, 2017 at 8:31 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> It is useful to have moved lines colored, but there are annoying corner >> cases, such as a single line moved, that is very common. For example >> in a typical patch of C code, we have closing braces that end statement >> blocks or functions. >> >> While it is technically true that these lines are moved as they show up >> elsewhere, it is harmful for the review as the reviewers attention is >> drawn to such a minor side annoyance. >> >> One of the first solutions considered, started off by these hypothesis': > > Hypotheses is the plural form of that word, I think. > >> (a) The more blocks of the same code we have, the less interesting it is. >> (b) The shorter a block of moved code is the less need of markup there >> is for review. >> >> Introduce a heuristic which drops any potential moved blocks if their >> length is shorter than the number of potential moved blocks. >> >> This heuristic was chosen as it is agnostic of the content (in other >> languages or contents to manage, we may have longer lines, e.g. in >> shell the closing of a condition is already 2 characters. Thinking >> about Latex documents tracked in Git, there can also be some >> boilerplate code with lots of characters) while taking both >> hypothesis' into account. An alternative considered was the number >> of non-whitespace characters in a line for example. > > It was puzzling what the above two paragraphs were. I took (a) and > (b) were the hypotheses, and the two above, and also the next > paragraphs, were the design that fell out of them. But that is not > what is happening. You changed your mind and settled on the design > in the next paragraph. Yes, I somehow want to say: "What is implemented in this patch is stupid. And I know it, but I know no smarter idea. This is what I thought was smarter, maybe someone in the future can be inspired by this, at least." > Perhaps we can do without all of the "I thought about this but it > didn't make sense" that is longer than the solution in the patch? As I do changes based on your responses, I want to squash these patches sent out last night into the original patch, so I'll butcher the commit message to be way smaller
Re: [PATCH 2/6] diff.c: change the default for move coloring to zebra
On Tue, Jun 27, 2017 at 8:14 PM, Junio C Hamano wrote:
>> diff --git a/diff.h b/diff.h
>> index 98abd75521..9298d211d7 100644
>> --- a/diff.h
>> +++ b/diff.h
>> @@ -192,6 +192,7 @@ struct diff_options {
>> COLOR_MOVED_NO = 0,
>> COLOR_MOVED_PLAIN = 1,
>> COLOR_MOVED_ZEBRA = 2,
>> + COLOR_MOVED_DEFAULT = 2,
>> COLOR_MOVED_ZEBRA_DIM = 3,
>> } color_moved;
>> };
>
> Hmph. I would have expected that COLOR_MOVED_DEFAULT would not be
> part of the enum, but a
>
> #define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
>
> I do not have a strong preference either way; it was just a bit
> unexpected.
Yes it is not as one would expect.
Originally I thought it could buy us some more protection via
smart static analysis (to be invented yet), but now I am not so
sure any more. Changed it to the #define.
Thanks.
Re: [GSoC][PATCH 1/6 v2] submodule--helper: introduce for_each_submodule_list
Prathamesh Chavan writes: > Introduce function for_each_submodule_list for using it > in the later patches, related to porting submodule > subcommands from shell to C. > This new function is also used in ported submodule subcommand > init. The patch text looks sensible. It would be easier for "git log" readers to understand, if the change is explained like so: Introduce function for_each_submodule_list() and replace a loop in module_init() with a call to it. The new function will also be used in other parts of the system in later patches. That way, readers do not have to judge the merit of this change based on a vague promise "it will help world better with future patches", but can instead judge on its immediate benefit that it refactors a useful bit out of an existing code. > Mentored-by: Christian Couder > Mentored-by: Stefan Beller > Signed-off-by: Prathamesh Chavan > --- > This series of patches is based on the 'next' branch. The reason not to base on 'master' is...? The thing is that a topic built on 'next' cannot be merged down to 'master' until _all_ other topics in 'next' graduate to 'master', which may never happen. If you are depending on one or more topics, please make sure to name them. Then we can (1) create a branch from the tip of 'master'; (2) merge these topics you depend on into that branch; and then (3) apply these patches. The topic still needs to wait until these other topis graduate, but at least you would not be blocked by unrelated topics that way. You _might_ be building on 'next' because you want to make sure that your topic works not just with master but also want to make sure that there won't be any unexpected breakage when used with topics in 'next', even though your topic does not depend on anything in 'next' in particular. It is a good development discipline to pay attention to other topics in flight and I applaud you for it if that is why you based it on 'next'. But the right way to do it would be to build your topic on 'master', and then in addition to testing the topic by itself, also make a trial merge of your topic into 'next' and test the result as well. Thanks.
Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list
Jeff Hostetler writes: > Yes, my logic was a little confusing there. Jonathan Tan said > something similar the other day. I have a new version that I'm > working on now that looks like this: > > list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_SHOW; > ... > if (filter) > r = filter(obj, path->buf, ... > if (r & LOFR_MARK_SEEN) > obj->flags |= SEEN; > if (r & LOFR_SHOW) > show(obj, path->buf, cb_data); > > I'm generalizing it a little to let the filter return 2 flags: > () SEEN to indicate that the filter doesn't want to see it again > () SHOW to include the object in the result. > These let filters do "hard" and "provisional" omits. (This will > make more sense later when I get my patch cleaned up.) It is not immediately obvious to me, especially without seeing the actual patch, why MARK_SEEN is needed. Especially given that I think a call to show() must set obj->flags |= SEEN anyway to avoid duplicate output, with or without the objects-filter mechanism. But that question can and should wait. > Yes, I'm including similar logic inside process_tree() to allow that > and let the filter know about entering and leaving each tree. So we > only need one filter-proc to handle a particular strategy and it will > handle both tree and blob objects. > > I want to be able to use this mechanism to do narrow clone/fetch > using such a filter-proc and a sparse-checkout-like spec. Good to know ;-).
Re: [PATCH/RFC v1 1/1] cygwin: Allow pushing to UNC paths
tbo...@web.de writes:
> From: Torsten Bögershausen
>
> cygwin can use an UNC path like //server/share/repo
> $ cd //server/share/dir
> $ mkdir test
> $ cd test
> $ git init --bare
>
> However, when we try to push from a local Git repository to this repo,
> there are 2 problems:
> - Git converts the leading "//" into a single "/".
> - The remote repo is not accepted because setup.c calls
> access(getenv(DB_ENVIRONMENT), X_OK)
> and this call fails. In other words, checking the executable bit
> of a directory mounted on a SAMBA share is not reliable (and not needed).
>
> As cygwin handles an UNC path so well, Git can support them better.
> - Introduce cygwin_offset_1st_component() which keeps the leading "//",
> similar to what Git for Windows does.
> - Move CYGWIN out of the POSIX in the tests for path normalization in t0060.
> - Use cygwin_access() with a relaxed test for the executable bit on
> a directory pointed out by an UNC path.
Thanks.
The offset-1st-component thing looks like a right thing to do.
I think the reason why you marked this as RFC is because you found
the "access" bit a bit iffy? If so, I share the feeling. If it
were called only from the codepath in setup.c::is_git_directory(),
it may be OK, but I suspect that there are other places that do care
about access() for other reasons in the codebase, and I am not sure
if it is safe to change the behaviour of access() like this.
Stepping back a bit.
The implementation of is_git_directory() wants to ensure that the
top level of the object database (i.e. $GIT_OBJECT_DIRECTORY or
$GIT_DIR/objects) and the reference store (i.e. $GIT_DIR/refs) can
be "executed". But what it really wants to see is that it is a
directory we can search. If we had a regular file that is executable,
it would happily say "Yes!", even though that is clearly bogus and
not a Git repository.
So perhaps we would want a bit higher-level abstraction API
implemented as:
int is_searchable_directory(const char *path)
{
struct stat st;
return (!stat(path, &st) && S_ISDIR(st.st_mode));
}
on Cygwin (as SMB share may not give you correct access(2)), and
int is_searchable_directory(const char *path)
{
struct stat st;
return (!stat(path, &st) &&
S_ISDIR(st.st_mode) &&
!access(path, X_OK));
}
elsewhere, or something like that, and use that in the
implementation of is_git_directory()?
I dunno. I see compat/mingw.c discards X_OK the same way you did,
so perhaps your version is a right solution at least in the shorter
term anyway.
Regardless, I think that we would want to make sure that the thing
is a directory where is_git_directory() uses access(2). But that
could be an orthogonal issue.
> Signed-off-by: Torsten Bögershausen
> ---
> compat/cygwin.c | 29 +
> compat/cygwin.h | 7 +++
> config.mak.uname | 1 +
> git-compat-util.h | 3 +++
> t/t0060-path-utils.sh | 2 ++
> 5 files changed, 42 insertions(+)
> create mode 100644 compat/cygwin.c
> create mode 100644 compat/cygwin.h
>
> diff --git a/compat/cygwin.c b/compat/cygwin.c
> new file mode 100644
> index 000..d98e877
> --- /dev/null
> +++ b/compat/cygwin.c
> @@ -0,0 +1,29 @@
> +#include "../git-compat-util.h"
> +#include "../cache.h"
> +
> +int cygwin_offset_1st_component(const char *path)
> +{
> + const char *pos = path;
> + /* unc paths */
> + if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
> + /* skip server name */
> + pos = strchr(pos + 2, '/');
> + if (!pos)
> + return 0; /* Error: malformed unc path */
> +
> + do {
> + pos++;
> + } while (*pos && pos[0] != '/');
> + }
> + return pos + is_dir_sep(*pos) - path;
> +}
> +
> +#undef access
> +int cygwin_access(const char *filename, int mode)
> +{
> + /* the execute bit does not work on SAMBA drives */
> + if (filename[0] == '/' && filename[1] == '/' )
> + return access(filename, mode & ~X_OK);
> + else
> + return access(filename, mode);
> +}
> diff --git a/compat/cygwin.h b/compat/cygwin.h
> new file mode 100644
> index 000..efa12ad
> --- /dev/null
> +++ b/compat/cygwin.h
> @@ -0,0 +1,7 @@
> +int cygwin_access(const char *filename, int mode);
> +#undef access
> +#define access cygwin_access
> +
> +
> +int cygwin_offset_1st_component(const char *path);
> +#define offset_1st_component cygwin_offset_1st_component
> diff --git a/config.mak.uname b/config.mak.uname
> index adfb90b..551e465 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -184,6 +184,7 @@ ifeq ($(uname_O),Cygwin)
> UNRELIABLE_FSTAT = UnfortunatelyYes
> SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
> OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
>
[PATCH] t1450: use egrep for regexp "alternation"
GNU grep allows "\(A\|B\)" as alternation in BRE, but this is an
extension not understood by some other implementations of grep
(Michael Kebe reported an breakage on Solaris).
Rewrite the offending test to ERE and use egrep instead.
Noticed-by: Michael Kebe
Signed-off-by: Junio C Hamano
---
t/t1450-fsck.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 8f52da2771..30e217dea2 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -541,7 +541,7 @@ test_expect_success 'fsck --name-objects' '
remove_loose_object $(git rev-parse julius:caesar.t) &&
test_must_fail git fsck --name-objects >out &&
tree=$(git rev-parse --verify julius:) &&
- grep "$tree (\(refs/heads/master\|HEAD\)@{[0-9]*}:" out
+ egrep "$tree \((refs/heads/master|HEAD)@\{[0-9]*\}:" out
)
'
--
2.13.2-655-gbf99380098
Re: Solaris 11.3 SPARC grep problem with t1450-fsck.sh
Michael Kebe writes:
> 2017-06-27 18:25 GMT+02:00 Junio C Hamano :
>> Ah, wait, that particular grep may have GNUism. If you changed it
>> to
>>
>> egrep "$tree \((refs/heads/master|HEAD)@{[0-9]*}:" out
>>
>> does it make it pass for you?
>
> Yes, this is working.
Thanks.
Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list
On 6/28/2017 12:23 PM, Junio C Hamano wrote:
Jeff Hostetler writes:
diff --git a/list-objects.c b/list-objects.c
index f3ca6aa..c9ca81c 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -24,11 +25,28 @@ static void process_blob(struct rev_info *revs,
die("bad blob object");
if (obj->flags & (UNINTERESTING | SEEN))
return;
- obj->flags |= SEEN;
pathlen = path->len;
strbuf_addstr(path, name);
- show(obj, path->buf, cb_data);
+ if (!filter_blob) {
+ /*
+* Normal processing is to imediately dedup blobs
+* during commit traversal, regardless of how many
+* times it appears in a single or multiple commits,
+* so we always set SEEN.
+*/
+ obj->flags |= SEEN;
+ show(obj, path->buf, cb_data);
+ } else {
+ /*
+* Use the filter-proc to decide whether to show
+* the blob. We only set SEEN if requested. For
+* example, this could be used to omit a specific
+* blob until it appears with a ".git*" entryname.
+*/
+ if (filter_blob(obj, path->buf, &path->buf[pathlen], cb_data))
+ obj->flags |= SEEN;
+ }
This somehow looks a bit surprising organization and division of
responsibility. I would have expected
if (!filter_blob ||
filter_blob(obj, path->buf, &path->buf[pathlen], cb_data) {
obj->flags |= SEEN;
show(obj, path->buf, cb_data);
}
i.e. making the filter function responsible for only making a
decision to include or exclude, not giving it a chance to decide to
"show" anything different.
Yes, my logic was a little confusing there. Jonathan Tan said
something similar the other day. I have a new version that I'm
working on now that looks like this:
list_objects_filter_result r = LOFR_MARK_SEEN | LOFR_SHOW;
...
if (filter)
r = filter(obj, path->buf, ...
if (r & LOFR_MARK_SEEN)
obj->flags |= SEEN;
if (r & LOFR_SHOW)
show(obj, path->buf, cb_data);
I'm generalizing it a little to let the filter return 2 flags:
() SEEN to indicate that the filter doesn't want to see it again
() SHOW to include the object in the result.
These let filters do "hard" and "provisional" omits. (This will
make more sense later when I get my patch cleaned up.)
@@ -67,6 +85,7 @@ static void process_gitlink(struct rev_info *revs,
static void process_tree(struct rev_info *revs,
struct tree *tree,
show_object_fn show,
+filter_blob_fn filter_blob,
struct strbuf *base,
const char *name,
void *cb_data)
@@ -111,7 +130,7 @@ static void process_tree(struct rev_info *revs,
if (S_ISDIR(entry.mode))
process_tree(revs,
lookup_tree(entry.oid->hash),
-show, base, entry.path,
+show, filter_blob, base, entry.path,
cb_data);
else if (S_ISGITLINK(entry.mode))
process_gitlink(revs, entry.oid->hash,
I wonder if we'll need filter_tree_fn in the future in this
codepath. When somebody wants to do a "narrow fetch/clone", would
the approach taken by this series, i.e. decide not to show certain
objects during the "rev-list --objects" traversal, a good precedent
to follow? Would this approach be a good foundation to build on
such a future?
Yes, I'm including similar logic inside process_tree() to allow that
and let the filter know about entering and leaving each tree. So we
only need one filter-proc to handle a particular strategy and it will
handle both tree and blob objects.
I want to be able to use this mechanism to do narrow clone/fetch
using such a filter-proc and a sparse-checkout-like spec.
Thanks,
Jeff
Re: [PATCH] commit-template: improve readability of commit template
Kaartic Sivaraam writes:
> The commit template adds the optional parts without
> a new line to distinguish them. This results in
> difficulty in interpreting it's content. Add new
> lines to separate the distinct parts of the template.
>
> The warning about usage of 'explicit paths without
> any corresponding options' has outlived it's purpose of
> warning users about the usage '--only' as default rather
> than '--include'. Remove it.
>
> Signed-off-by: Kaartic Sivaraam
> ---
> builtin/commit.c | 9 +
> 1 file changed, 1 insertion(+), 8 deletions(-)
I think I can agree with both of the changes, but this commit does
two things that may be better done in two separate commits. If I
were doing this patch, I probably would make [PATCH 1/2] about
removing the only_include_assumed (which is all except for the hunk
at -877,8) and then [PATCH 2/2] about giving a separating blank line
before the "git status" output.
> The warning about usage of 'explicit paths without
> any corresponding options' has outlived it's purpose of
> warning users about the usage '--only' as default rather
> than '--include'. Remove it.
With a bit more digging of the history:
The notice that "git commit " default to "git commit
--only " was there since 756e3ee0 ("Merge branch
'jc/commit'", 2006-02-14). Back then, existing users of Git
expected the command doing "git commit --include ", and
after we changed the behaviour of the command to align with
other people's "$scm commit ", we added the text to help
them transition their expectations. Remove the message that now
has outlived its usefulness.
perhaps.
Thanks.
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8d1cac062..22d17e6f2 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -139,7 +139,6 @@ static enum commit_whence whence;
> static int sequencer_in_use;
> static int use_editor = 1, include_status = 1;
> static int show_ignored_in_status, have_option_m;
> -static const char *only_include_assumed;
> static struct strbuf message = STRBUF_INIT;
>
> static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
> @@ -841,9 +840,6 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> "with '%c' will be kept; you may remove them"
> " yourself if you want to.\n"
> "An empty message aborts the commit.\n"),
> comment_line_char);
> - if (only_include_assumed)
> - status_printf_ln(s, GIT_COLOR_NORMAL,
> - "%s", only_include_assumed);
>
> /*
>* These should never fail because they come from our own
> @@ -877,8 +873,7 @@ static int prepare_to_commit(const char *index_file,
> const char *prefix,
> (int)(ci.name_end - ci.name_begin),
> ci.name_begin,
> (int)(ci.mail_end - ci.mail_begin),
> ci.mail_begin);
>
> - if (ident_shown)
> - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
> + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new
> line for clarity */
>
> saved_color_setting = s->use_color;
> s->use_color = 0;
> @@ -1208,8 +1203,6 @@ static int parse_and_validate_options(int argc, const
> char *argv[],
> die(_("Only one of --include/--only/--all/--interactive/--patch
> can be used."));
> if (argc == 0 && (also || (only && !amend && !allow_empty)))
> die(_("No paths with --include/--only does not make sense."));
> - if (argc > 0 && !also && !only)
> - only_include_assumed = _("Explicit paths specified without -i
> or -o; assuming --only paths...");
> if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
> cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
> else if (!strcmp(cleanup_arg, "verbatim"))
[PATCH/RFC v1 1/1] cygwin: Allow pushing to UNC paths
From: Torsten Bögershausen
cygwin can use an UNC path like //server/share/repo
$ cd //server/share/dir
$ mkdir test
$ cd test
$ git init --bare
However, when we try to push from a local Git repository to this repo,
there are 2 problems:
- Git converts the leading "//" into a single "/".
- The remote repo is not accepted because setup.c calls
access(getenv(DB_ENVIRONMENT), X_OK)
and this call fails. In other words, checking the executable bit
of a directory mounted on a SAMBA share is not reliable (and not needed).
As cygwin handles an UNC path so well, Git can support them better.
- Introduce cygwin_offset_1st_component() which keeps the leading "//",
similar to what Git for Windows does.
- Move CYGWIN out of the POSIX in the tests for path normalization in t0060.
- Use cygwin_access() with a relaxed test for the executable bit on
a directory pointed out by an UNC path.
Signed-off-by: Torsten Bögershausen
---
compat/cygwin.c | 29 +
compat/cygwin.h | 7 +++
config.mak.uname | 1 +
git-compat-util.h | 3 +++
t/t0060-path-utils.sh | 2 ++
5 files changed, 42 insertions(+)
create mode 100644 compat/cygwin.c
create mode 100644 compat/cygwin.h
diff --git a/compat/cygwin.c b/compat/cygwin.c
new file mode 100644
index 000..d98e877
--- /dev/null
+++ b/compat/cygwin.c
@@ -0,0 +1,29 @@
+#include "../git-compat-util.h"
+#include "../cache.h"
+
+int cygwin_offset_1st_component(const char *path)
+{
+ const char *pos = path;
+ /* unc paths */
+ if (is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
+ /* skip server name */
+ pos = strchr(pos + 2, '/');
+ if (!pos)
+ return 0; /* Error: malformed unc path */
+
+ do {
+ pos++;
+ } while (*pos && pos[0] != '/');
+ }
+ return pos + is_dir_sep(*pos) - path;
+}
+
+#undef access
+int cygwin_access(const char *filename, int mode)
+{
+ /* the execute bit does not work on SAMBA drives */
+ if (filename[0] == '/' && filename[1] == '/' )
+ return access(filename, mode & ~X_OK);
+ else
+ return access(filename, mode);
+}
diff --git a/compat/cygwin.h b/compat/cygwin.h
new file mode 100644
index 000..efa12ad
--- /dev/null
+++ b/compat/cygwin.h
@@ -0,0 +1,7 @@
+int cygwin_access(const char *filename, int mode);
+#undef access
+#define access cygwin_access
+
+
+int cygwin_offset_1st_component(const char *path);
+#define offset_1st_component cygwin_offset_1st_component
diff --git a/config.mak.uname b/config.mak.uname
index adfb90b..551e465 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -184,6 +184,7 @@ ifeq ($(uname_O),Cygwin)
UNRELIABLE_FSTAT = UnfortunatelyYes
SPARSE_FLAGS = -isystem /usr/include/w32api -Wno-one-bit-signed-bitfield
OBJECT_CREATION_USES_RENAMES = UnfortunatelyNeedsTo
+ COMPAT_OBJS += compat/cygwin.o
endif
ifeq ($(uname_S),FreeBSD)
NEEDS_LIBICONV = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index 047172d..db9c22d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -189,6 +189,9 @@
#include
#endif
+#if defined(__CYGWIN__)
+#include "compat/cygwin.h"
+#endif
#if defined(__MINGW32__)
/* pull in Windows compatibility stuff */
#include "compat/mingw.h"
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 444b5a4..7ea2bb5 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -70,6 +70,8 @@ ancestor() {
case $(uname -s) in
*MINGW*)
;;
+*CYGWIN*)
+ ;;
*)
test_set_prereq POSIX
;;
--
2.10.0
Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list
Jeff Hostetler writes:
> diff --git a/list-objects.c b/list-objects.c
> index f3ca6aa..c9ca81c 100644
> --- a/list-objects.c
> +++ b/list-objects.c
> @@ -24,11 +25,28 @@ static void process_blob(struct rev_info *revs,
> die("bad blob object");
> if (obj->flags & (UNINTERESTING | SEEN))
> return;
> - obj->flags |= SEEN;
>
> pathlen = path->len;
> strbuf_addstr(path, name);
> - show(obj, path->buf, cb_data);
> + if (!filter_blob) {
> + /*
> + * Normal processing is to imediately dedup blobs
> + * during commit traversal, regardless of how many
> + * times it appears in a single or multiple commits,
> + * so we always set SEEN.
> + */
> + obj->flags |= SEEN;
> + show(obj, path->buf, cb_data);
> + } else {
> + /*
> + * Use the filter-proc to decide whether to show
> + * the blob. We only set SEEN if requested. For
> + * example, this could be used to omit a specific
> + * blob until it appears with a ".git*" entryname.
> + */
> + if (filter_blob(obj, path->buf, &path->buf[pathlen], cb_data))
> + obj->flags |= SEEN;
> + }
This somehow looks a bit surprising organization and division of
responsibility. I would have expected
if (!filter_blob ||
filter_blob(obj, path->buf, &path->buf[pathlen], cb_data) {
obj->flags |= SEEN;
show(obj, path->buf, cb_data);
}
i.e. making the filter function responsible for only making a
decision to include or exclude, not giving it a chance to decide to
"show" anything different.
> @@ -67,6 +85,7 @@ static void process_gitlink(struct rev_info *revs,
> static void process_tree(struct rev_info *revs,
>struct tree *tree,
>show_object_fn show,
> + filter_blob_fn filter_blob,
>struct strbuf *base,
>const char *name,
>void *cb_data)
> @@ -111,7 +130,7 @@ static void process_tree(struct rev_info *revs,
> if (S_ISDIR(entry.mode))
> process_tree(revs,
>lookup_tree(entry.oid->hash),
> - show, base, entry.path,
> + show, filter_blob, base, entry.path,
>cb_data);
> else if (S_ISGITLINK(entry.mode))
> process_gitlink(revs, entry.oid->hash,
I wonder if we'll need filter_tree_fn in the future in this
codepath. When somebody wants to do a "narrow fetch/clone", would
the approach taken by this series, i.e. decide not to show certain
objects during the "rev-list --objects" traversal, a good precedent
to follow? Would this approach be a good foundation to build on
such a future?
Thanks.
Re: [PATCH/RFC] commit-template: improve readability of commit template
I might have been ignorant about something about git in my reply in the previous email (found below). In that case, please enlighten me. On Wed, 2017-06-28 at 18:34 +0530, Kaartic Sivaraam wrote: > On Tue, 2017-06-27 at 10:56 -0700, Junio C Hamano wrote: > > Kaartic Sivaraam writes: > > > I thought it's not good to trade-off readability for vertical > > > space > > > as > > > the ultimate aim of the commit template (at least to me) is to > > > convey > > > information to the user about the commit that he's going to make. > > > For > > > which, I thought it made more sense to improve it's readability > > > by > > > adding new lines between different sections rather than constrain > > > the > > > output within a few lines. > > > > You have to be careful when making a trade-off argument. It > > depends > > on how familiar you already are with the presentation. Those who > > are/got used to the order of things that come, they will know there > > is extra information when the block of lines are longer than usual > > without reading every character and then their eyes are guided to > > read what is extra, without having to waste precious screen real > > estate. Nobody will _stay_ a new user who is not yet familiar with > > the everyday output. > > > > You're right. I didn't consider the fact that experienced users would > be affected as a result of this change, sorry about that. I thought, > making this change would help the new users who would possibly find > the > commit template to be congested and let experienced users to get > accustomed to this new output format. I thought this change would be > a > win-win (at least after people get accustomed to the new > formatting). > > In case screen real estate is considered more important here, no > issues. I'll drop that part of the change, happily. > > > > I actually didn't think of modifying that in order to keep it in > > > line > > > with the output of `git status`. > > > > I was (and still am) assuming that if we make this change to "git > > commit", we should make matching change to "git status" as a given. > > I get it now. In that case, I don't think making the change would be > a > good choice for the following reasons, > > * I think vertical spacing matters more in the output printed to > a > console. > * I myself find it odd to add a new line below the branch > information possibly because I'm too accustomed to it's current > output. > > I tried adding the new line, it seemed to be too spacious. It might > be > just me in this case. > > > > Further, to me, adding *this* new line > > > before the "Changes not staged for commit" (or something in it's > > > place) > > > seems to be wasting some vertical space ... > > > > I think it is in line with your original reasoning why you wanted > > these extra blank lines to separate blocks of different kinds of > > information: > > > > - "Please do this" instruction at the beginning > > - Make sure you know the default is --only, not --include > > - By the way you are committing for that person, not you > > - This change is being committed on that branch > > - Here are the changes that are already in the index > > - Here are the changes that are not in the index > > - Here are untracked files > > > > Lack of a blank between the fourth block and the fifth block [*1*] > > makes it somewhat inconsistent, doesn't it? > > > > It does, for the given set of blocks. I didn't find it inconsistent > as > I thought the separate blocks as follows, > > - "Please do this" instruction at the beginning > - Make sure you know the default is --only, not --include > - By the way you are committing for that person, not you > - Status of repository (git status) > > > [Footnote] > > > > *1* Yes, we should think about removing the optional second block, > > as I think that it outlived its usefulness; if we are to do so, > > these become the third and the fourth blocks. > > If I interpreted your previous email correctly, I thought we were > doing > it! > > I'll send a "typical" patch as a follow-up of this mail. >
Re: [PATCH] commit-template: improve readability of commit template
I forgot to add the RFC in the subject in a hurry. Please consider it's presence. -- Regards, Kaartic Sivaraam
Re: Bug: Cannot kill Nodejs process using ctrl + c
Hi, On Mon, 26 Jun 2017, Gyandeep Singh wrote: > 3. hit "CTRL + c" (2 times) to kill the process. > 4. If you look at taskmanager, then you will see a node.js process > still running. or if you try to restart the server it will say port > 300 already in use. The way Ctrl+C is handled in Git for Windows changed recently, indeed. Remember: sending signals to processes to ask them to terminate is *POSIX* semantics. Not Windows semantics. On Windows you can terminate a process via the Win32 API. And I really mean "terminate". There is no chance for that process to, say, remove a now-stale .git/index.lock. We have to jump through hoops to accomodate Git's lack of non-POSIX understandings. In this particular case, we inject a remote thread into the process, running ExitProcess() so that the atexit() handlers have a chance to perform the cleanup that Git so cleverly expects to be able to do when being terminated. It is probably that thread that is still trying to run when you hit Ctrl+C the second time, and that second time it terminates *just* the shadow process: node.exe is a Console program, and since you run it in Git Bash (which does not have a Win32 Console, but emulates a Unix terminal instead) the node shell alias runs node.exe through a helper called winpty.exe that I suspect gets terminated without taking down its child processes. Ciao, Johannes
[PATCH] commit-template: improve readability of commit template
The commit template adds the optional parts without
a new line to distinguish them. This results in
difficulty in interpreting it's content. Add new
lines to separate the distinct parts of the template.
The warning about usage of 'explicit paths without
any corresponding options' has outlived it's purpose of
warning users about the usage '--only' as default rather
than '--include'. Remove it.
Signed-off-by: Kaartic Sivaraam
---
builtin/commit.c | 9 +
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 8d1cac062..22d17e6f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -139,7 +139,6 @@ static enum commit_whence whence;
static int sequencer_in_use;
static int use_editor = 1, include_status = 1;
static int show_ignored_in_status, have_option_m;
-static const char *only_include_assumed;
static struct strbuf message = STRBUF_INIT;
static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
@@ -841,9 +840,6 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
"with '%c' will be kept; you may remove them"
" yourself if you want to.\n"
"An empty message aborts the commit.\n"),
comment_line_char);
- if (only_include_assumed)
- status_printf_ln(s, GIT_COLOR_NORMAL,
- "%s", only_include_assumed);
/*
* These should never fail because they come from our own
@@ -877,8 +873,7 @@ static int prepare_to_commit(const char *index_file, const
char *prefix,
(int)(ci.name_end - ci.name_begin),
ci.name_begin,
(int)(ci.mail_end - ci.mail_begin),
ci.mail_begin);
- if (ident_shown)
- status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
+ status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new
line for clarity */
saved_color_setting = s->use_color;
s->use_color = 0;
@@ -1208,8 +1203,6 @@ static int parse_and_validate_options(int argc, const
char *argv[],
die(_("Only one of --include/--only/--all/--interactive/--patch
can be used."));
if (argc == 0 && (also || (only && !amend && !allow_empty)))
die(_("No paths with --include/--only does not make sense."));
- if (argc > 0 && !also && !only)
- only_include_assumed = _("Explicit paths specified without -i
or -o; assuming --only paths...");
if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
else if (!strcmp(cleanup_arg, "verbatim"))
--
2.11.0
Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes
Thanks Luke,
regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT
should display error), for me it's very weird too as it doesn't seem
to be related to this particular change, as the patch changes are not
exercised with that test.
The test 21 in t9807 was precisely the new test added to test the
change (it was passing with local setup), the test log is truncated
before the output of test 21 in t9807 but I'm afraid I'm not very
familiar with Travis, so maybe I'm missing something. Is there a way
to have the full logs or they are always truncated after some number
of lines?
I think you get an error with git diff --check because I added spaces
after a tab, but those spaces are intentional, the tabs are for the
"<<-EOF" and spaces are for the "p4 triggers" specificiation.
Thanks,
On Wed, Jun 28, 2017 at 11:54 AM, Luke Diamand wrote:
> On 28 June 2017 at 05:08, Junio C Hamano wrote:
>> Miguel Torroja writes:
>>
>>> The option -G of p4 (python marshal output) gives more context about the
>>> data being output. That's useful when using the command "change -o" as
>>> we can distinguish between warning/error line and real change description.
>>>
>>> Some p4 triggers in the server side generate some warnings when
>>> executed. Unfortunately those messages are mixed with the output of
>>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>>> in python marshal output (-G). The real change output is reported as
>>> {'code':'stat'}
>>>
>>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
>>> that outputs extra lines with "p4 change -o" and "p4 changes"
>>>
>>> Signed-off-by: Miguel Torroja
>>> ---
>>
>> It appears that https://travis-ci.org/git/git/builds/247724639
>> does not like this change. For example:
>>
>> https://travis-ci.org/git/git/jobs/247724642#L1848
>>
>> indicates that not just 9807 (new tests added by this patch) but
>> also 9800 starts to fail.
>>
>> I'd wait for git-p4 experts to comment and help guiding this change
>> forward.
>
> I only see a (very weird) failure in t9800. I wonder if there are some
> P4 version differences.
>
> Client: Rev. P4/LINUX26X86_64/2015.1/1024208 (2015/03/16).
> Server: P4D/LINUX26X86_64/2015.1/1028542 (2015/03/20)
>
> There's also a whitespace error according to "git diff --check".
> :
> Sadly I don't think there's any way to do this and yet keep the "#
> edit" comments. It looks like "p4 change -o" outputs lines with "'#
> edit" on the end, but the (supposedly semantically equivalent) "p4 -G
> change -o" command does not. I think that's a P4 bug.
>
> So we have a choice of fixing a garbled message in the face of scripts
> in the backend, or keeping the comments, or writing some extra Python
> to infer them. I vote for fixing the garbled message.
>
> Luke
Re: [PATCH/RFC] commit-template: improve readability of commit template
On Tue, 2017-06-27 at 10:56 -0700, Junio C Hamano wrote: > Kaartic Sivaraam writes: > > I thought it's not good to trade-off readability for vertical space > > as > > the ultimate aim of the commit template (at least to me) is to > > convey > > information to the user about the commit that he's going to make. > > For > > which, I thought it made more sense to improve it's readability by > > adding new lines between different sections rather than constrain > > the > > output within a few lines. > > You have to be careful when making a trade-off argument. It depends > on how familiar you already are with the presentation. Those who > are/got used to the order of things that come, they will know there > is extra information when the block of lines are longer than usual > without reading every character and then their eyes are guided to > read what is extra, without having to waste precious screen real > estate. Nobody will _stay_ a new user who is not yet familiar with > the everyday output. > You're right. I didn't consider the fact that experienced users would be affected as a result of this change, sorry about that. I thought, making this change would help the new users who would possibly find the commit template to be congested and let experienced users to get accustomed to this new output format. I thought this change would be a win-win (at least after people get accustomed to the new formatting). In case screen real estate is considered more important here, no issues. I'll drop that part of the change, happily. > > I actually didn't think of modifying that in order to keep it in > > line > > with the output of `git status`. > > I was (and still am) assuming that if we make this change to "git > commit", we should make matching change to "git status" as a given. I get it now. In that case, I don't think making the change would be a good choice for the following reasons, * I think vertical spacing matters more in the output printed to a console. * I myself find it odd to add a new line below the branch information possibly because I'm too accustomed to it's current output. I tried adding the new line, it seemed to be too spacious. It might be just me in this case. > > Further, to me, adding *this* new line > > before the "Changes not staged for commit" (or something in it's > > place) > > seems to be wasting some vertical space ... > > I think it is in line with your original reasoning why you wanted > these extra blank lines to separate blocks of different kinds of > information: > > - "Please do this" instruction at the beginning > - Make sure you know the default is --only, not --include > - By the way you are committing for that person, not you > - This change is being committed on that branch > - Here are the changes that are already in the index > - Here are the changes that are not in the index > - Here are untracked files > > Lack of a blank between the fourth block and the fifth block [*1*] > makes it somewhat inconsistent, doesn't it? > It does, for the given set of blocks. I didn't find it inconsistent as I thought the separate blocks as follows, - "Please do this" instruction at the beginning - Make sure you know the default is --only, not --include - By the way you are committing for that person, not you - Status of repository (git status) > [Footnote] > > *1* Yes, we should think about removing the optional second block, > as I think that it outlived its usefulness; if we are to do so, > these become the third and the fourth blocks. If I interpreted your previous email correctly, I thought we were doing it! I'll send a "typical" patch as a follow-up of this mail. -- Regards, Kaartic Sivaraam
Re: Bug: submodules of submodules use absolute gitdir in .git file (instead of relative)
I had the problem with version 2.7.4. The problem is fixed in self-compiled v2.13.2.533.ge0aaa1bed, v2.12.0, v2.11.0 and v2.9.3! Even the older version 2.1.4 does not have that problem. Thank you for the input! Robert From: Siemer Robert CSNPE Sent: Wednesday, June 28, 2017 14:09 To: Stefan Beller; Junio C Hamano Cc: git@vger.kernel.org Subject: Re: Bug: submodules of submodules use absolute gitdir in .git file (instead of relative) From: Stefan Beller Sent: Tuesday, June 27, 2017 19:35 To: Junio C Hamano Cc: Siemer Robert CSNPE; git@vger.kernel.org Subject: Re: Bug: submodules of submodules use absolute gitdir in .git file (instead of relative) On Tue, Jun 27, 2017 at 8:06 AM, Junio C Hamano wrote: > Robert Siemer writes: > >> Hello everyone, >> >> $ git submodule foreach cat .git >> >> ...shows that the gitdir registered in the .git file of submodules is >> relative. But if you run >> >> $ git submodule foreach --recursive cat .git >> >> ...shows that submodules of submodules (if you have any) have an absolute >> gitdir starting with a slash. >> >> 1) Can you confirm that behavior? > > Nobody can without knowing which version of Git you are using. I > suspect that we had discussed and addressed something related to > nested submodule's "gitdir" pointer in not-so-distant past. Yeah there was a period in time where we had this bug. (Or is it a different bug?) See 90c0011619 (submodule: use absolute path for computing relative path connecting, 2016-12-08), which is included in 2.12.
Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes
On 28 June 2017 at 05:08, Junio C Hamano wrote:
> Miguel Torroja writes:
>
>> The option -G of p4 (python marshal output) gives more context about the
>> data being output. That's useful when using the command "change -o" as
>> we can distinguish between warning/error line and real change description.
>>
>> Some p4 triggers in the server side generate some warnings when
>> executed. Unfortunately those messages are mixed with the output of
>> "p4 change -o". Those extra warning lines are reported as {'code':'info'}
>> in python marshal output (-G). The real change output is reported as
>> {'code':'stat'}
>>
>> A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
>> that outputs extra lines with "p4 change -o" and "p4 changes"
>>
>> Signed-off-by: Miguel Torroja
>> ---
>
> It appears that https://travis-ci.org/git/git/builds/247724639
> does not like this change. For example:
>
> https://travis-ci.org/git/git/jobs/247724642#L1848
>
> indicates that not just 9807 (new tests added by this patch) but
> also 9800 starts to fail.
>
> I'd wait for git-p4 experts to comment and help guiding this change
> forward.
I only see a (very weird) failure in t9800. I wonder if there are some
P4 version differences.
Client: Rev. P4/LINUX26X86_64/2015.1/1024208 (2015/03/16).
Server: P4D/LINUX26X86_64/2015.1/1028542 (2015/03/20)
There's also a whitespace error according to "git diff --check".
:
Sadly I don't think there's any way to do this and yet keep the "#
edit" comments. It looks like "p4 change -o" outputs lines with "'#
edit" on the end, but the (supposedly semantically equivalent) "p4 -G
change -o" command does not. I think that's a P4 bug.
So we have a choice of fixing a garbled message in the face of scripts
in the backend, or keeping the comments, or writing some extra Python
to infer them. I vote for fixing the garbled message.
Luke
Re: [PATCH 1/1] ls-remote: remove "-h" from help text
Hi Jonathan,
> > This regression was fixed in 91a640ffb6d9 ("ls-remote: a lone "-h" is
> > [...]
>
> Without this patch, I'm able to run
>
> git ls-remote -h .
>
> This patch removes that support. Intended?
*hihi* okay it was to counter-intuitive for me. "91a640ffb6d" talks about this
issue.
Thanks for enlightenment :-)
Silvio
--
-- S. Fricke sil...@port1024.net --
Diplom-Informatiker (FH)
Linux-Development Matrix: @silvio:port1024.net
enquiry 28-06-17
Hello, My name is Ms Julian Smith and i am from Sinara Group Co.Ltd in Russia..We are glad to know about your company from the web and we are interested in your products.Please send us your Latest catalog and price list for our trial order Julian Smith, Purchasing Manager
Re: Bug: Since Git 2.13.2 git rebase autostash doesn't use fast-forward (poor performance)
Could you see this as suggestion for performance improvement or should I send another e-mail for a new task? Regards, Mattias Junio C Hamano schrieb am 27.06.2017 17:02:14: > Von: Junio C Hamano > An: neul...@dakosy.de > Kopie: git@vger.kernel.org > Datum: 27.06.2017 17:02 > Betreff: Re: Bug: Since Git 2.13.2 git rebase autostash doesn't use > fast-forward (poor performance) > Gesendet von: Junio C Hamano > > neul...@dakosy.de writes: > > > since the latest version 2.13.2 "git pull --rebase --autostash" doesn't > > use a fast forward if possible. > > This may not be a bug but instead a fix made deliberately. > > cf. http://public-inbox.org/git/ > caazatrcaob7exvrcvc9rkmo02g5xcp8gpbajefhfv7zaxvp...@mail.gmail.com/ > > A deciding excerpt from the thread was: > > > Correctness must trump optimizations > > Patches to further update and resurrect the optimization without > breaking correctness of course are of course very much welcomed. > > Thanks.
