Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Junio C Hamano]

Duy Nguyen  writes:

> On Thu, Jan 26, 2017 at 3:57 AM, Jeff King  wrote:
>> I don't think it means either. It means to include remotes in the
>> selected revisions, but excluding the entries mentioned by --exclude.
>>
>> IOW:
>>
>>   --exclude=foo --remotes
>> include all remotes except refs/remotes/foo
>>
>>   --exclude=foo --unrelated --remotes
>> same
>>
>>   --exclude=foo --decorate-reflog --remotes
>> decorate reflogs of all remotes except "foo". Do _not_ use them
>> as traversal tips.
>>
>>   --decorate-reflog --exclude=foo --remotes
>> same
>>
>> IOW, the ref-selector options build up until a group option is given,
>> which acts on the built-up options (over that group) and then resets the
>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>> think in practice nobody would do that, because it's hard to read).
>
> This is because it makes sense to combine --exclude and
> --decorate-reflog. But what about a new --something that conflicts
> with either --exclude or --decorate-reflog?

I would think that "--exclude=foo --something --remotes" 

 * should be diagnosed as an error if "--something" is not compatible
   with "--exclude";

 * should take effect at the concluding "--remotes" if "--something"
   is similar to "--decorate-reflog" whose effect ends at a
   concluding --remotes/--branches/etc.; and

 * should work independently if "--something" is neither.

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jeff King]

On Thu, Jan 26, 2017 at 04:28:17PM +0700, Duy Nguyen wrote:

> On Thu, Jan 26, 2017 at 3:57 AM, Jeff King  wrote:
> > I don't think it means either. It means to include remotes in the
> > selected revisions, but excluding the entries mentioned by --exclude.
> >
> > IOW:
> >
> >   --exclude=foo --remotes
> > include all remotes except refs/remotes/foo
> >
> >   --exclude=foo --unrelated --remotes
> > same
> >
> >   --exclude=foo --decorate-reflog --remotes
> > decorate reflogs of all remotes except "foo". Do _not_ use them
> > as traversal tips.
> >
> >   --decorate-reflog --exclude=foo --remotes
> > same
> >
> > IOW, the ref-selector options build up until a group option is given,
> > which acts on the built-up options (over that group) and then resets the
> > built-up options. Doing "--unrelated" as above is orthogonal (though I
> > think in practice nobody would do that, because it's hard to read).
> 
> This is because it makes sense to combine --exclude and
> --decorate-reflog. But what about a new --something that conflicts
> with either --exclude or --decorate-reflog? Should we simply catch
> such combinations and error out (which may be a bit more complicated
> than this patch, or maybe not)?

I'd cross that bridge when we see what the option is. But my gut is that
rules would be:

  - apply all non-conflicting relevant options. So:

  --exclude=foo/* --decorate-refs --decorate-reflog --remotes

would presumably decorate both ref tips _and_ reflogs for all
remotes (except ones in refs/remotes/foo/*)

  - for ones that are directly related and override each other,
use the usual last-one-wins rule. So:

  --decorate-reflog --no-decorate-reflog --remotes

would countermand the original --decorate-reflog.

  - for ones that really have complex interactions, notice and complain
in handle_refs().

That just seems to me like it follows our usual option parsing
procedure. The only difference here is that process and reset some
subset of the flags when we hit a special marker option ("--remotes" in
these examples) instead of doing it at the end.

-Peff

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Duy Nguyen]

On Thu, Jan 26, 2017 at 3:57 AM, Jeff King  wrote:
> I don't think it means either. It means to include remotes in the
> selected revisions, but excluding the entries mentioned by --exclude.
>
> IOW:
>
>   --exclude=foo --remotes
> include all remotes except refs/remotes/foo
>
>   --exclude=foo --unrelated --remotes
> same
>
>   --exclude=foo --decorate-reflog --remotes
> decorate reflogs of all remotes except "foo". Do _not_ use them
> as traversal tips.
>
>   --decorate-reflog --exclude=foo --remotes
> same
>
> IOW, the ref-selector options build up until a group option is given,
> which acts on the built-up options (over that group) and then resets the
> built-up options. Doing "--unrelated" as above is orthogonal (though I
> think in practice nobody would do that, because it's hard to read).

This is because it makes sense to combine --exclude and
--decorate-reflog. But what about a new --something that conflicts
with either --exclude or --decorate-reflog? Should we simply catch
such combinations and error out (which may be a bit more complicated
than this patch, or maybe not)?
-- 
Duy

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Junio C Hamano]

Jacob Keller  writes:

> On Wed, Jan 25, 2017 at 1:27 PM, Jeff King  wrote:
>> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:
>>
>>> IOW, the ref-selector options build up until a group option is given,
>>> which acts on the built-up options (over that group) and then resets the
>>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>>> think in practice nobody would do that, because it's hard to read).
>>
>> So here's what I would have expected your series to look more like (with
>> probably one patch adding clear_ref_selection_options, and the other
>> adding the decorate stuff):
>>
>
> I agree that this is how I would have expected it to work as well.

That makes three of us ;-)

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jacob Keller]

On Wed, Jan 25, 2017 at 1:27 PM, Jeff King  wrote:
> On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:
>
>> IOW, the ref-selector options build up until a group option is given,
>> which acts on the built-up options (over that group) and then resets the
>> built-up options. Doing "--unrelated" as above is orthogonal (though I
>> think in practice nobody would do that, because it's hard to read).
>
> So here's what I would have expected your series to look more like (with
> probably one patch adding clear_ref_selection_options, and the other
> adding the decorate stuff):
>

I agree that this is how I would have expected it to work as well.

Thanks,
Jake

> diff --git a/revision.c b/revision.c
> index b37dbec37..2f67707c7 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const 
> struct object_id *oid,
>
> if (ref_excluded(cb->all_revs->ref_excludes, path))
> return 0;
> +   if (cb->all_revs->decorate_reflog) {
> +   /* TODO actually do it for real */
> +   warning("would decorate %s", path);
> +   return 0; /* do not add it as a tip */
> +   }
>
> object = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);
> add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, 
> cb->all_flags);
> @@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list 
> **ref_excludes_p, const char *exclude)
> string_list_append(*ref_excludes_p, exclude);
>  }
>
> +static void clear_ref_selection_options(struct rev_info *revs)
> +{
> +   clear_ref_exclusion(>ref_excludes);
> +   revs->decorate_reflog = 0;
> +}
> +
>  static void handle_refs(const char *submodule, struct rev_info *revs, 
> unsigned flags,
> int (*for_each)(const char *, each_ref_fn, void *))
>  {
> @@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char 
> *submodule,
> if (!strcmp(arg, "--all")) {
> handle_refs(submodule, revs, *flags, for_each_ref_submodule);
> handle_refs(submodule, revs, *flags, head_ref_submodule);
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (!strcmp(arg, "--branches")) {
> handle_refs(submodule, revs, *flags, 
> for_each_branch_ref_submodule);
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (!strcmp(arg, "--bisect")) {
> read_bisect_terms(_bad, _good);
> handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
> @@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char 
> *submodule,
> revs->bisect = 1;
> } else if (!strcmp(arg, "--tags")) {
> handle_refs(submodule, revs, *flags, 
> for_each_tag_ref_submodule);
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (!strcmp(arg, "--remotes")) {
> handle_refs(submodule, revs, *flags, 
> for_each_remote_ref_submodule);
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if ((argcount = parse_long_opt("glob", argv, ))) {
> struct all_refs_cb cb;
> init_all_refs_cb(, revs, *flags);
> for_each_glob_ref(handle_one_ref, optarg, );
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> return argcount;
> } else if ((argcount = parse_long_opt("exclude", argv, ))) {
> add_ref_exclusion(>ref_excludes, optarg);
> @@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char 
> *submodule,
> struct all_refs_cb cb;
> init_all_refs_cb(, revs, *flags);
> for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", 
> );
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (starts_with(arg, "--tags=")) {
> struct all_refs_cb cb;
> init_all_refs_cb(, revs, *flags);
> for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", 
> );
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> } else if (starts_with(arg, "--remotes=")) {
> struct all_refs_cb cb;
> init_all_refs_cb(, revs, *flags);
> for_each_glob_ref_in(handle_one_ref, arg + 10, 
> "refs/remotes/", );
> -   clear_ref_exclusion(>ref_excludes);
> +   clear_ref_selection_options(revs);
> +   } else if (!strcmp(arg, "--decorate-reflog")) {
> +   revs->decorate_reflog = 1;
> } else if 

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jeff King]

On Wed, Jan 25, 2017 at 03:57:18PM -0500, Jeff King wrote:

> IOW, the ref-selector options build up until a group option is given,
> which acts on the built-up options (over that group) and then resets the
> built-up options. Doing "--unrelated" as above is orthogonal (though I
> think in practice nobody would do that, because it's hard to read).

So here's what I would have expected your series to look more like (with
probably one patch adding clear_ref_selection_options, and the other
adding the decorate stuff):

diff --git a/revision.c b/revision.c
index b37dbec37..2f67707c7 100644
--- a/revision.c
+++ b/revision.c
@@ -1156,6 +1156,11 @@ static int handle_one_ref(const char *path, const struct 
object_id *oid,
 
if (ref_excluded(cb->all_revs->ref_excludes, path))
return 0;
+   if (cb->all_revs->decorate_reflog) {
+   /* TODO actually do it for real */
+   warning("would decorate %s", path);
+   return 0; /* do not add it as a tip */
+   }
 
object = get_reference(cb->all_revs, path, oid->hash, cb->all_flags);
add_rev_cmdline(cb->all_revs, object, path, REV_CMD_REF, cb->all_flags);
@@ -1188,6 +1193,12 @@ void add_ref_exclusion(struct string_list 
**ref_excludes_p, const char *exclude)
string_list_append(*ref_excludes_p, exclude);
 }
 
+static void clear_ref_selection_options(struct rev_info *revs)
+{
+   clear_ref_exclusion(>ref_excludes);
+   revs->decorate_reflog = 0;
+}
+
 static void handle_refs(const char *submodule, struct rev_info *revs, unsigned 
flags,
int (*for_each)(const char *, each_ref_fn, void *))
 {
@@ -2080,10 +2091,10 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
if (!strcmp(arg, "--all")) {
handle_refs(submodule, revs, *flags, for_each_ref_submodule);
handle_refs(submodule, revs, *flags, head_ref_submodule);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (!strcmp(arg, "--branches")) {
handle_refs(submodule, revs, *flags, 
for_each_branch_ref_submodule);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (!strcmp(arg, "--bisect")) {
read_bisect_terms(_bad, _good);
handle_refs(submodule, revs, *flags, for_each_bad_bisect_ref);
@@ -2091,15 +2102,15 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
revs->bisect = 1;
} else if (!strcmp(arg, "--tags")) {
handle_refs(submodule, revs, *flags, 
for_each_tag_ref_submodule);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (!strcmp(arg, "--remotes")) {
handle_refs(submodule, revs, *flags, 
for_each_remote_ref_submodule);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if ((argcount = parse_long_opt("glob", argv, ))) {
struct all_refs_cb cb;
init_all_refs_cb(, revs, *flags);
for_each_glob_ref(handle_one_ref, optarg, );
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
return argcount;
} else if ((argcount = parse_long_opt("exclude", argv, ))) {
add_ref_exclusion(>ref_excludes, optarg);
@@ -2108,17 +2119,19 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
struct all_refs_cb cb;
init_all_refs_cb(, revs, *flags);
for_each_glob_ref_in(handle_one_ref, arg + 11, "refs/heads/", 
);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (starts_with(arg, "--tags=")) {
struct all_refs_cb cb;
init_all_refs_cb(, revs, *flags);
for_each_glob_ref_in(handle_one_ref, arg + 7, "refs/tags/", 
);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
} else if (starts_with(arg, "--remotes=")) {
struct all_refs_cb cb;
init_all_refs_cb(, revs, *flags);
for_each_glob_ref_in(handle_one_ref, arg + 10, "refs/remotes/", 
);
-   clear_ref_exclusion(>ref_excludes);
+   clear_ref_selection_options(revs);
+   } else if (!strcmp(arg, "--decorate-reflog")) {
+   revs->decorate_reflog = 1;
} else if (!strcmp(arg, "--reflog")) {
add_reflogs_to_pending(revs, *flags);
} else if (!strcmp(arg, "--indexed-objects")) {
diff --git a/revision.h b/revision.h
index 9fac1a607..c74879829 100644
--- a/revision.h
+++ b/revision.h
@@ -66,6 +66,8 @@ struct rev_info {
/* excluding from --branches, --refs, etc. expansion */

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
>
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
>
> --exclude .. --decorate-reflog ... --remotes
>
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?

I would expect that the effect of exclude, decorate-reflog and
friends will extend until the next occurrence of --remotes (or --all
or whatever you catch in parse_ref_selector_option() function).

So, I'd read it as "add all remote tracking refs, but (1) exclude
exclude the refs matching pattern .. and (2) use reflog of them if
they match pattern ...".

Or did you mean by "..." something other than a single argument that
is a pattern?

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jeff King]

On Wed, Jan 25, 2017 at 07:50:53PM +0700, Nguyễn Thái Ngọc Duy wrote:

> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
> 
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
> 
> --exclude .. --decorate-reflog ... --remotes
> 
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?

I don't think it means either. It means to include remotes in the
selected revisions, but excluding the entries mentioned by --exclude.

IOW:

  --exclude=foo --remotes
include all remotes except refs/remotes/foo

  --exclude=foo --unrelated --remotes
same

  --exclude=foo --decorate-reflog --remotes
decorate reflogs of all remotes except "foo". Do _not_ use them
as traversal tips.

  --decorate-reflog --exclude=foo --remotes
same

IOW, the ref-selector options build up until a group option is given,
which acts on the built-up options (over that group) and then resets the
built-up options. Doing "--unrelated" as above is orthogonal (though I
think in practice nobody would do that, because it's hard to read).

> Granted, there may be valid use cases for such a combination (e.g.
> "decorate all reflogs except remote ones") but I feel option order is
> not a good fit to express them.

That would be spelled:

  --exclude=refs/remotes --decorate-reflogs --all

(or you could swap the first two options).

Again, I'm not sure if I'm missing something subtle, or if you are
confused about how --exclude works. :)

-Peff

Re: [PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Jacob Keller]

On Wed, Jan 25, 2017 at 4:50 AM, Nguyễn Thái Ngọc Duy  wrote:
> Behavior change: "--exclude --blah --remotes" will not exclude remote
> branches any more. Only "--exclude --remotes" does.
>
> This is because --exclude is going to have a new friend --decorate-reflog
> who haves the same way. When you allow a distant --remotes to complement
> a previous option, things get complicated. In
>
> --exclude .. --decorate-reflog ... --remotes
>
> Does it mean decorate remote reflogs, or does it mean exclude remotes
> from the selected revisions?
>
> Granted, there may be valid use cases for such a combination (e.g.
> "decorate all reflogs except remote ones") but I feel option order is
> not a good fit to express them.
>

Limiting the scope of the exclude seems somewhat reasonable to me,
because it makes it much easier to explain and show the user. We do
need to make sure it's not going to break any scripts or other issues.
Is it possible for us to produce an error if the user does "--exclude"
without a necessary connecting option?

Thanks,
Jake

[PATCH 4/5] revision.c: refactor ref selection handler after --exclude

[Nguyễn Thái Ngọc Duy]

Behavior change: "--exclude --blah --remotes" will not exclude remote
branches any more. Only "--exclude --remotes" does.

This is because --exclude is going to have a new friend --decorate-reflog
who haves the same way. When you allow a distant --remotes to complement
a previous option, things get complicated. In

--exclude .. --decorate-reflog ... --remotes

Does it mean decorate remote reflogs, or does it mean exclude remotes
from the selected revisions?

Granted, there may be valid use cases for such a combination (e.g.
"decorate all reflogs except remote ones") but I feel option order is
not a good fit to express them.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 revision.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index cda2606c66..45cffcab44 100644
--- a/revision.c
+++ b/revision.c
@@ -2152,10 +2152,24 @@ static int handle_refs_pseudo_opt(const char *submodule,
return 0;
}
 
-   clear_ref_exclusion(>ref_excludes);
return 1;
 }
 
+static int handle_revision_pseudo_opt(const char *, struct rev_info *, int, 
const char **, int *);
+
+static int handle_revision_pseudo_opt_after_exclude(const char *submodule,
+   struct rev_info *revs,
+   int argc, const char **argv,
+   int *flags)
+{
+   int ret;
+
+   ret = handle_revision_pseudo_opt(submodule, revs, argc, argv, flags);
+   clear_ref_exclusion(>ref_excludes);
+   revs->handle_pseudo_opt = NULL;
+   return ret;
+}
+
 static int handle_revision_pseudo_opt(const char *submodule,
struct rev_info *revs,
int argc, const char **argv, int *flags)
@@ -2184,6 +2198,7 @@ static int handle_revision_pseudo_opt(const char 
*submodule,
revs->bisect = 1;
} else if ((argcount = parse_long_opt("exclude", argv, ))) {
add_ref_exclusion(>ref_excludes, optarg);
+   revs->handle_pseudo_opt = 
handle_revision_pseudo_opt_after_exclude;
return argcount;
} else if (!strcmp(arg, "--reflog")) {
add_reflogs_to_pending(revs, *flags);
-- 
2.11.0.157.gd943d85