| Message ID | 20231019193911.1669705-1-andy.koppe@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | log: decorate pseudorefs and other refs | expand |
Andy Koppe <andy.koppe@gmail.com> writes: > This patch series adds three slots to the color.decorate.<slot> config > option: > - 'symbol' for coloring the punctuation symbols used around the refs in > decorations, which currently use the same color as the commit hash. > - 'ref' for coloring refs other than branches, remote-tracking branches, > tags and the stash, which currently are not colored when included in > decorations through custom decoration filter options. > - 'pseudoref' for coloring pseudorefs such as ORIG_HEAD or MERGE_HEAD. > Include them in decorations by default. > > This series is to replace the 'decorate: add color.decorate.symbols > config option' patch proposed at: > https://lore.kernel.org/git/20231003205442.22963-1-andy.koppe@gmail.com If that is the case, it probably would have been nicer to mark the series as [PATCH v2]. Also, can you make messages [1/7]..[7/7] replies to [0/7] when you send them out? It seems that all 8 of them (including the cover letter) are replies to the previous round, which looked a bit unusual. As to the contents of the series: [1/7] nicely lays out the color documentation; I do not think the extra verbosity was absolutely needed for existing ones (e.g., when a reader sees 'tag', the reader knows the color will be applied to tags), but the more exotic ones the series will be adding may deserve extra explanation on what they are, so I guess it is OK. [2/7] is a trivial readability improvement. It obviously should be left outside the scope of this series, but we should notice the same pattern in similar color tables (e.g., wt-status.c has one, diff.c has another) and perform the same clean-up as a #leftoverbits item. [3/7] They way _NIL color is used to control the defaulting looked a bit unusual, but clever way to use a non-constant color defined elsewhere as its default. A similar trick is used in wt-status.c:color() for STATUS_ONBRANCH, so this is nothing new. [4/7] The name of new member .include added to ref_namespace_info will not be understood by anybody unless they are too deeply obsessed by decoration mechansim. As the namespace_info covers far wider interest, so a name that *shouts* that it is about decoration filter must be used to be understood by readers of the code. To be quite honest, "decoration filter" is probably a name that will not be understood by anybody, but coming up with a better name for it is probably outside the scope of this series. [5/7] I am not sure if "other refs" should be an item in the namespace_info array. If it is truly "catch-all", then shouldn't the refs in other namespaces without their own decoration (e.g. ones in refs/notes/ and refs/prefetch/) be colored in the same way as this new class? And if so, having it as an independent element that sits next to these other classes smells like a strange design. Another more worrying thing is that existing .ref members are designed to never overlap with each other, but this one obviously does. When a caller with a ref (or a pseudoref) asks "which namespace does this one belong to", does the existing code still do the right thing with this new element? Without it, because there was no overlap, an implementation can randomly search in the namespace_info table and stop at the first hit, but now with the overlapping and widely open .ref = "refs/", the implementation of the search must know that it is a fallback position (i.e. if it found a match with the fallback .ref = "refs/" , unless it looked at all other entries that could begin with "refs/" and are more specific, it needs to keep going). [6/7] This is pretty straight-forward, assuming that the existing is_pseudoref_syntax() function does the right thing. I am not sure about that, though. A refname with '-' is allowed to be called a pseudoref??? Also, not a fault of this patch, but the "_syntax" in its name is totally unnecessary, I would think. At first glance, I suspected that the excuse to append _syntax may have been to signal the fact that the helper function does not check if there actually is such a ref, but examining a few helpers defined nearby tells us that such an excuse does not make sense: int is_per_worktree_ref(const char *) { return starts_with(refname, "refs/worktree/") || starts_with(refname, "refs/bisect/") || starts_with(refname, "refs/rewritten/"); } int is_pseudoref_syntax(const char *); int is_current_worktree_ref(const char *ref) { return is_pseudoref_syntax(ref) || is_per_worktree_ref(ref); } All these three work on the refname and based on what is in that refname string, decides what kind of ref it is. There is nothing especially "syntax" about the second one, and we should rename it as part of #leftoverbits clean-up effort. Another unrelated tangent is that is_per_worktree_ref() shown above and the namespace_info array we saw earlier are not even aware of each other, which is maintenance nightmare waiting to happen. [7/7] Allowing pseudorefs to optionally used when decorating might be a good idea, but I do not think it is particularly a good design decision to enable it by default. Each of them forming a separate "namespace" also looks like a poor design, as being able to group multiple things into one family and treat them the same way is the primary point of "namespace", I would think. You do not want to say "I want to decorate off of ORIG_HEAD and FETCH_HEAD"; instead you would want to say "I want to decorate off of any pseudoref".
On 22/10/2023 01:13, Junio C Hamano wrote: > Andy Koppe <andy.koppe@gmail.com> writes: >> This series is to replace the 'decorate: add color.decorate.symbols >> config option' patch proposed at: >> https://lore.kernel.org/git/20231003205442.22963-1-andy.koppe@gmail.com > > If that is the case, it probably would have been nicer to mark the > series as [PATCH v2]. Thanks, I wasn't sure about that due to the change in title and increase in scope. I shall err towards version-bumping in any future such cases. > Also, can you make messages [1/7]..[7/7] replies to [0/7] when you > send them out? It seems that all 8 of them (including the cover > letter) are replies to the previous round, which looked a bit > unusual. Not quite sure how that happened, but I think my mistake was passing --in-reply-to to git-format-patch instead of git-send-email. > [2/7] is a trivial readability improvement. It obviously should be > left outside the scope of this series, but we should notice > the same pattern in similar color tables (e.g., wt-status.c > has one, diff.c has another) and perform the same clean-up as > a #leftoverbits item. Okay, I've removed that commit in v2. (I should have mentioned in the commit message that it was triggered by the inconsistency with the immediately following color_decorate_slots array, which uses designated initializers.) > [4/7] The name of new member .include added to ref_namespace_info > will not be understood by anybody unless they are too deeply > obsessed by decoration mechansim. As the namespace_info > covers far wider interest, so a name that *shouts* that it is > about decoration filter must be used to be understood by > readers of the code Agreed. > [5/7] I am not sure if "other refs" should be an item in the > namespace_info array. If it is truly "catch-all", then > shouldn't the refs in other namespaces without their own > decoration (e.g. ones in refs/notes/ and refs/prefetch/) be > colored in the same way as this new class? They would, because add_ref_decoration() skips ref_namespace entries without a decoration type, so they would fall through to "refs/" and pick up the DECORATION_REF type. > And if so, having > it as an independent element that sits next to these other > classes smells like a strange design. > > Another more worrying thing is that existing .ref members are > designed to never overlap with each other, but this one > obviously does. When a caller with a ref (or a pseudoref) > asks "which namespace does this one belong to", does the > existing code still do the right thing with this new element? > Without it, because there was no overlap, an implementation > can randomly search in the namespace_info table and stop at > the first hit, but now with the overlapping and widely open > .ref = "refs/", the implementation of the search must know > that it is a fallback position (i.e. if it found a match with > the fallback .ref = "refs/" , unless it looked at all other > entries that could begin with "refs/" and are more specific, > it needs to keep going). Fair points. I've rewritten things to not touch the ref_namespace array. > [6/7] This is pretty straight-forward, assuming that the existing > is_pseudoref_syntax() function does the right thing. I am > not sure about that, though. A refname with '-' is allowed > to be called a pseudoref??? > > Also, not a fault of this patch, but the "_syntax" in its > name is totally unnecessary, I would think. At first glance, > I suspected that the excuse to append _syntax may have been > to signal the fact that the helper function does not check if > there actually is such a ref, but examining a few helpers > defined nearby tells us that such an excuse does not make > sense: I've dropped the use of that function from the change, checking against the actual pseudoref names instead. > [7/7] Allowing pseudorefs to optionally used when decorating might > be a good idea, but I do not think it is particularly a good > design decision to enable it by default. Okay! > Each of them forming a separate "namespace" also looks like a > poor design, as being able to group multiple things into one > family and treat them the same way is the primary point of > "namespace", I would think. Fair enough, although the array already contains HEAD and refs/stash as singletons. I had vacillated about shoe-horning the pseudorefs in there, and was swayed by having a single place to define which (pseudo)refs should be included in decorations by default. That motivation goes away with all the pseudorefs off by default. I've rewritten things to handle the pseudorefs separately from the ref_namespace array, with iteration functions similar to the ones used for HEAD and proper refs. > You do not want to say "I want > to decorate off of ORIG_HEAD and FETCH_HEAD"; instead you > would want to say "I want to decorate off of any pseudoref". They can now all be enabled with --clear-decorations or log.initialDecorationSet=all, or be controlled individually with the other filter options. Thank you very much for the review! Andy
Andy Koppe <andy.koppe@gmail.com> writes: >> [2/7] is a trivial readability improvement. It obviously should be >> left outside the scope of this series, but we should notice >> the same pattern in similar color tables (e.g., wt-status.c >> has one, diff.c has another) and perform the same clean-up as >> a #leftoverbits item. > > Okay, I've removed that commit in v2. (I should have mentioned in the > commit message that it was triggered by the inconsistency with the > immediately following color_decorate_slots array, which uses > designated initializers.) Sorry, that is not what I meant. [2/7] as a preliminary clean-up to work in the same area does make very much sense. What I meant to be "outside the scope" was to make similar fixes to other color tables that this series does not care about. >> .ref = "refs/", the implementation of the search must know >> that it is a fallback position (i.e. if it found a match with >> the fallback .ref = "refs/" , unless it looked at all other >> entries that could begin with "refs/" and are more specific, >> it needs to keep going). > > Fair points. I've rewritten things to not touch the ref_namespace array. Well, the namespace_info mechanism still may be a good place to have the necessary information; it may be that the current implementation detail of how a given ref is classified to one of the namespaces is too limiting---it essentially allows the string match with the .ref member. But we can imagine that it could be extended a bit, e.g. struct ref_namespace_info { char *ref; int (*membership)(const char *, const struct ref_namespace_info *); ... other members ...; }; where the .membership member is used in add_ref_decoration() to determine the membership of a given "refname" to the namespace "i" perhaps like so: struct ref_namespace_info *info = &ref_namespace[i]; if (!info->decoration) continue; + if (info->membership) { + if (info->membership(refname, info)) { + deco_type = info->decoration; + break; + } + } else if (info->exact) { - if (info->exact) { if (!strcmp(refname, info->ref)) { deco_type = info_decoration; break; } Then you can arrange the pseudoref class to use .membership function perhaps like this: static int pseudoref_namespace_membership( const char *refname, const struct ref_namespace_info *info UNUSED ) { return is_pseudoref(refname); } and make them all into a single class. What I called a bad design was to reuse the namespace_info code without extending it to suit our needs. This comment will probably affect everything below. >> [6/7] This is pretty straight-forward, assuming that the existing >> is_pseudoref_syntax() function does the right thing. I am >> not sure about that, though. A refname with '-' is allowed >> to be called a pseudoref??? >> Also, not a fault of this patch, but the "_syntax" in its >> name is totally unnecessary, I would think. At first glance, >> I suspected that the excuse to append _syntax may have been >> to signal the fact that the helper function does not check if >> there actually is such a ref, but examining a few helpers >> defined nearby tells us that such an excuse does not make >> sense: > > I've dropped the use of that function from the change, checking > against the actual pseudoref names instead. > >> [7/7] Allowing pseudorefs to optionally used when decorating might >> be a good idea, but I do not think it is particularly a good >> design decision to enable it by default. > > Okay! > >> Each of them forming a separate "namespace" also looks like a >> poor design, as being able to group multiple things into one >> family and treat them the same way is the primary point of >> "namespace", I would think. > > Fair enough, although the array already contains HEAD and refs/stash > as singletons. But these deserve to be singletons, don't they? There is no other thing that behaves like HEAD; there is no other thing that behaves like stash; and they do not behave like each other. Having said that, I do not think it makes much sense to decorate a commit off of refs/stash, as the true richeness of the stash is not in its history but in its reflog, which the decoration code does not dig into. But obviously it is not a part of the topic we are discussing (unless, of course, we are not "adding" new decoration sources and colors, but we are improving the decoration sources and colors by adding new useful ones while retiring existing useless ones). Thanks.
On 23/10/2023 01:20, Junio C Hamano wrote: > Andy Koppe <andy.koppe@gmail.com> writes: > >>> [2/7] is a trivial readability improvement. It obviously should be >>> left outside the scope of this series, but we should notice >>> the same pattern in similar color tables (e.g., wt-status.c >>> has one, diff.c has another) and perform the same clean-up as >>> a #leftoverbits item. >> >> Okay, I've removed that commit in v2. (I should have mentioned in the >> commit message that it was triggered by the inconsistency with the >> immediately following color_decorate_slots array, which uses >> designated initializers.) > > Sorry, that is not what I meant. [2/7] as a preliminary clean-up to > work in the same area does make very much sense. What I meant to be > "outside the scope" was to make similar fixes to other color tables > that this series does not care about. Ah, sorry for misreading. Commit reinstated in v3. >> Fair enough, although the array already contains HEAD and refs/stash >> as singletons. > > But these deserve to be singletons, don't they? There is no other > thing that behaves like HEAD; there is no other thing that behaves > like stash; and they do not behave like each other. They do indeed, but arguably the pseudorefs are singletons rather than a namespace like refs/heads as well, as there is a defined and documented set of them. >> I've rewritten things to not touch the ref_namespace array. > > Well, the namespace_info mechanism still may be a good place to have > the necessary information; it may be that the current implementation > detail of how a given ref is classified to one of the namespaces is > too limiting---it essentially allows the string match with the .ref > member. But we can imagine that it could be extended a bit, e.g. > > struct ref_namespace_info { > char *ref; > int (*membership)(const char *, const struct ref_namespace_info *); > ... other members ...; > }; > > where the .membership member is used in add_ref_decoration() to > determine the membership of a given "refname" to the namespace "i" > perhaps like so: > > struct ref_namespace_info *info = &ref_namespace[i]; > > if (!info->decoration) > continue; > + if (info->membership) { > + if (info->membership(refname, info)) { > + deco_type = info->decoration; > + break; > + } > + } else if (info->exact) { > - if (info->exact) { > if (!strcmp(refname, info->ref)) { > deco_type = info_decoration; > break; > } > > Then you can arrange the pseudoref class to use .membership function > perhaps like this: > > static int pseudoref_namespace_membership( > const char *refname, const struct ref_namespace_info *info UNUSED > ) > { > return is_pseudoref(refname); > } > > and make them all into a single class. That's an interesting idea, but I'm not convinced it would buy us much, while also potentially complicating things for any other uses of the ref_namespace array. My premise here is that we do need a list of the documented pseudorefs, so that we can iterate through them and add the ones that do exist to the decorations, whereby I admit that shoe-horning that list into the ref_namespace array wasn't a good idea. If that premise is wrong, and there's a better way to discover the pseudorefs, the following might be moot. Sending each found pseudoref through add_ref_decoration() and its lookup of ref_namespace would just confirm what we already know: it's a pseudoref. Which is why both my initial attempt and the current one don't actually invoke add_ref_decoration() for them. Could you have a closer look at the current design? It handles the pseudorefs separately from proper refs, with their own iteration and callback functions, which I think makes for simpler more self-contained changes than v1 or the approach suggested above. > Having said that, I do not think it makes much sense to decorate a > commit off of refs/stash, as the true richeness of the stash is not > in its history but in its reflog, which the decoration code does not > dig into. But obviously it is not a part of the topic we are > discussing (unless, of course, we are not "adding" new decoration > sources and colors, but we are improving the decoration sources and > colors by adding new useful ones while retiring existing useless > ones). I agree refs/stash is a weird one, and that it could be subsumed into the color.decoration.ref setting for 'refs/*' that I'm adding here, which is also why I chose the same default color for it. I'd be happy to drop color.decoration.stash if the minor break in compatibility for anyone who has customized it is acceptable. The setting would be quietly ignored. Another related thought: the '--clear-decorations' option of git-log seems unfortunately named as it suggests the opposite of what it actually does, which is to enable all decorations (unless subsequently constrained with '--decorate-refs{,--exclude}=...'). Regards, Andy