Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns
Jacob Keller writes: > On Wed, Jan 18, 2017 at 12:04 PM, Junio C Hamano wrote: > >> I agree that we cannot short-cut on the first match to make sure >> that the outcome is stable, but I wondered what should be shown when >> we do have multiple matches. Say I gave >> >> --refs="v*" --refs="refs/tags/v1.*" >> >> and refs/tags/v1.0 matched. The above code would say we can >> abbreviate. >> >> What is the reason behind this design decision? Is it because it is >> clear that the user shows her willingness to accept more compact >> form by having --refs="v*" that would allow shortening? If that is >> the case, I think I agree with the reasoning. But we probably want >> to write it down somewhere, because another reasoning, which may >> also be valid, would call for an opposite behaviour (i.e. the more >> specific --refs="refs/tags/v1.*" also matched, so let's show that >> fact by not shortening). > > I'm not sure which reasoning makes most sense. Any other opinions? FWIW, I do think that the design decision to declare that it can be abbreviated if the ref matches at least one short pattern makes sense, and I am guessing (because you didn't answer when asked what _your_ reasoning behind the code was) that you are in agreement. I just want it to be spelled out probably as in-code comment, so that people who later come to this part of the code know why it was designed that way. And they can disagree and change it if the end result is better---I just want to make sure that they can understand what they are disagreeing when it happens, as opposed to them scratching their head saying "we do not know why it was chosen to be done this way, let's make a random change to make it behave differently".
Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns
On Wed, Jan 18, 2017 at 12:04 PM, Junio C Hamano wrote:
> Jacob Keller writes:
>
>> From: Jacob Keller
>>
>> Teach git name-rev to take a string list of patterns from --refs instead
>> of only a single pattern. The list of patterns will be matched
>> inclusively, such that a ref only needs to match one pattern to be
>> included. If a ref will only be excluded if it does not match any of the
>> patterns.
>
> I think "If a" in the last sentence should be "A".
You are correct, that is a typo.
>
>> --refs=::
>> Only use refs whose names match a given shell pattern. The pattern
>> - can be one of branch name, tag name or fully qualified ref name.
>> + can be one of branch name, tag name or fully qualified ref name. If
>> + given multiple times, use refs whose names match any of the given shell
>> + patterns. Use `--no-refs` to clear any previous ref patterns given.
>
> Unlike 1/5, this is facing the end-users, and the last sentence is
> very appropriate.
Yes.
>
>> + if (data->ref_filters.nr) {
>> + struct string_list_item *item;
>> + int matched = 0;
>> +
>> + /* See if any of the patterns match. */
>> + for_each_string_list_item(item, &data->ref_filters) {
>> + /*
>> + * We want to check every pattern even if we already
>> + * found a match, just in case one of the later
>> + * patterns could abbreviate the output.
>> + */
>> + switch (subpath_matches(path, item->string)) {
>> + case -1: /* did not match */
>> + break;
>> + case 0: /* matched fully */
>> + matched = 1;
>> + break;
>> + default: /* matched subpath */
>> + matched = 1;
>> + can_abbreviate_output = 1;
>> + break;
>> + }
>> }
>
> I agree that we cannot short-cut on the first match to make sure
> that the outcome is stable, but I wondered what should be shown when
> we do have multiple matches. Say I gave
>
> --refs="v*" --refs="refs/tags/v1.*"
>
> and refs/tags/v1.0 matched. The above code would say we can
> abbreviate.
>
> What is the reason behind this design decision? Is it because it is
> clear that the user shows her willingness to accept more compact
> form by having --refs="v*" that would allow shortening? If that is
> the case, I think I agree with the reasoning. But we probably want
> to write it down somewhere, because another reasoning, which may
> also be valid, would call for an opposite behaviour (i.e. the more
> specific --refs="refs/tags/v1.*" also matched, so let's show that
> fact by not shortening).
>
I'm not sure which reasoning makes most sense. Any other opinions?
Thanks,
Jake
Re: [PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns
Jacob Keller writes:
> From: Jacob Keller
>
> Teach git name-rev to take a string list of patterns from --refs instead
> of only a single pattern. The list of patterns will be matched
> inclusively, such that a ref only needs to match one pattern to be
> included. If a ref will only be excluded if it does not match any of the
> patterns.
I think "If a" in the last sentence should be "A".
> --refs=::
> Only use refs whose names match a given shell pattern. The pattern
> - can be one of branch name, tag name or fully qualified ref name.
> + can be one of branch name, tag name or fully qualified ref name. If
> + given multiple times, use refs whose names match any of the given shell
> + patterns. Use `--no-refs` to clear any previous ref patterns given.
Unlike 1/5, this is facing the end-users, and the last sentence is
very appropriate.
> + if (data->ref_filters.nr) {
> + struct string_list_item *item;
> + int matched = 0;
> +
> + /* See if any of the patterns match. */
> + for_each_string_list_item(item, &data->ref_filters) {
> + /*
> + * We want to check every pattern even if we already
> + * found a match, just in case one of the later
> + * patterns could abbreviate the output.
> + */
> + switch (subpath_matches(path, item->string)) {
> + case -1: /* did not match */
> + break;
> + case 0: /* matched fully */
> + matched = 1;
> + break;
> + default: /* matched subpath */
> + matched = 1;
> + can_abbreviate_output = 1;
> + break;
> + }
> }
I agree that we cannot short-cut on the first match to make sure
that the outcome is stable, but I wondered what should be shown when
we do have multiple matches. Say I gave
--refs="v*" --refs="refs/tags/v1.*"
and refs/tags/v1.0 matched. The above code would say we can
abbreviate.
What is the reason behind this design decision? Is it because it is
clear that the user shows her willingness to accept more compact
form by having --refs="v*" that would allow shortening? If that is
the case, I think I agree with the reasoning. But we probably want
to write it down somewhere, because another reasoning, which may
also be valid, would call for an opposite behaviour (i.e. the more
specific --refs="refs/tags/v1.*" also matched, so let's show that
fact by not shortening).
[PATCH v3 2/5] name-rev: extend --refs to accept multiple patterns
From: Jacob Keller
Teach git name-rev to take a string list of patterns from --refs instead
of only a single pattern. The list of patterns will be matched
inclusively, such that a ref only needs to match one pattern to be
included. If a ref will only be excluded if it does not match any of the
patterns.
Add tests and documentation for this change. The tests do use
dynamically generated output as part of the expected output because the
expected output is the raw commit-id and it seemed like a bad idea to
hardcode that into a tests expected output.
Signed-off-by: Jacob Keller
---
Documentation/git-name-rev.txt | 4 +++-
builtin/name-rev.c | 41 +---
t/t6007-rev-list-cherry-pick-file.sh | 26 +++
3 files changed, 58 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index ca28fb8e2a07..7433627db12d 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -26,7 +26,9 @@ OPTIONS
--refs=::
Only use refs whose names match a given shell pattern. The pattern
- can be one of branch name, tag name or fully qualified ref name.
+ can be one of branch name, tag name or fully qualified ref name. If
+ given multiple times, use refs whose names match any of the given shell
+ patterns. Use `--no-refs` to clear any previous ref patterns given.
--all::
List all commits reachable from all refs
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index cd89d48b65e8..000a2a700ed3 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -108,7 +108,7 @@ static const char *name_ref_abbrev(const char *refname, int
shorten_unambiguous)
struct name_ref_data {
int tags_only;
int name_only;
- const char *ref_filter;
+ struct string_list ref_filters;
};
static struct tip_table {
@@ -150,16 +150,33 @@ static int name_ref(const char *path, const struct
object_id *oid, int flags, vo
if (data->tags_only && !starts_with(path, "refs/tags/"))
return 0;
- if (data->ref_filter) {
- switch (subpath_matches(path, data->ref_filter)) {
- case -1: /* did not match */
- return 0;
- case 0: /* matched fully */
- break;
- default: /* matched subpath */
- can_abbreviate_output = 1;
- break;
+ if (data->ref_filters.nr) {
+ struct string_list_item *item;
+ int matched = 0;
+
+ /* See if any of the patterns match. */
+ for_each_string_list_item(item, &data->ref_filters) {
+ /*
+* We want to check every pattern even if we already
+* found a match, just in case one of the later
+* patterns could abbreviate the output.
+*/
+ switch (subpath_matches(path, item->string)) {
+ case -1: /* did not match */
+ break;
+ case 0: /* matched fully */
+ matched = 1;
+ break;
+ default: /* matched subpath */
+ matched = 1;
+ can_abbreviate_output = 1;
+ break;
+ }
}
+
+ /* If none of the patterns matched, stop now */
+ if (!matched)
+ return 0;
}
add_to_tip_table(oid->hash, path, can_abbreviate_output);
@@ -306,11 +323,11 @@ int cmd_name_rev(int argc, const char **argv, const char
*prefix)
{
struct object_array revs = OBJECT_ARRAY_INIT;
int all = 0, transform_stdin = 0, allow_undefined = 1, always = 0,
peel_tag = 0;
- struct name_ref_data data = { 0, 0, NULL };
+ struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP };
struct option opts[] = {
OPT_BOOL(0, "name-only", &data.name_only, N_("print only names
(no SHA-1)")),
OPT_BOOL(0, "tags", &data.tags_only, N_("only use tags to name
the commits")),
- OPT_STRING(0, "refs", &data.ref_filter, N_("pattern"),
+ OPT_STRING_LIST(0, "refs", &data.ref_filters, N_("pattern"),
N_("only use refs matching ")),
OPT_GROUP(""),
OPT_BOOL(0, "all", &all, N_("list all commits reachable from
all refs")),
diff --git a/t/t6007-rev-list-cherry-pick-file.sh
b/t/t6007-rev-list-cherry-pick-file.sh
index 1408b608eb03..d9827a6389a3 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh
@@ -99,6 +99,32 @@ test_expect_success '--cherry-pick bar does not come up
empty (II)' '
