On Tue, 28 Oct 2008, Jeff King wrote:
>
> - Does it make sense to have this _in addition_ to --decorate (since
> for any commit with a --decorate field, it would likely be the same
> as --source)? Should it be a different type of decorate instead,
> like --decorate=source or --decorate=branch?
I think they are different. People who want --source generally have other
issues than people who want --decorate, and the two do actually work
together.
In particular, think about things like "gitk", which currently can't do
_either_, but that could easily support both. Even to the point where gitk
might want to add both flags on its own, just to always get the branch and
the decorate output.
And no, they are _not_ the same. They are vehemently not the same when you
use abything but '--all', and even with --all they are different because
decorate has no problems with multiple decorations on one commit, while
source is very much a single thing per commit.
And --source really has to be just a single data field, because anything
else will almost inevitably be too expensive to be worth it.
> - Should this be triggered by the "%d" --pretty=format specifier? This
> two-liner:
>
> diff --git a/pretty.c b/pretty.c
> index f6ff312..bdaad19 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -487,6 +487,8 @@ static void format_decoration(struct strbuf *sb, const struct commit *commit)
> const char *prefix = " (";
>
> load_ref_decorations();
> + if (commit->util)
> + printf("%s", (char *)commit->util);
> d = lookup_decoration(&name_decoration, &commit->object);
> while (d) {
> strbuf_addstr(sb, prefix);
>
> works, but:
>
> - it doesn't check revs->show_source, so is it possible that
> commit->util is being used for something else?
Indeed. You should always do the show_source check. There are different
things that use 'util', and while I don't think any of them will ever use
the format string, it's still a good idea to just make it consistent.
However:
> - using '%d' automatically turns on --decorate, so you end up with
> both the --source and --decorate values. More sensible semantics
> would be "%d turns on --decorate, unless you have done
> --decorate=<explicit format>".
As mentioned above, I think this is a non-starter. I don't think
"decorate" and "source" really have anything to do with each other, except
that they get printed out in similar ways and in the same function for the
default printout.
And quite frankly, even that was partly just a "minimal diff" thing,
although I do think that they both are "decorations", it's just that they
are very _different_ decorations.
> Alternatively, this should just be "%b" or "%S".
So yeah, I'd expect a new format specifier.
> - If you don't specify --all, you just get "HEAD" for everything.
> Which makes sense when you consider the implementation, but I think
> is probably a bit confusing for users.
I don't think it's at all confusing, for two reasons:
- you'd never ever use it manually unless you give multiple branches. Why
would you spend time to type out '--source' unless it was because you
needed to?
- for scripting, you want things consistent, even if the 'consistency' is
purely about always showing HEAD when that's the only source.
For the second case, imagine having gitk always add "--source" and
"--decorate" to the command line (the same way it always adds --parents
and --pretty=raw etc). gitk doesn't want to care how many branches you
give it as arguments, or whether you use --tags or --all. But it would
want to always parse things the same way.
No?
> Hmm. It would be nice to keep even a simple counter to get a "distance"
> from the ref and choose the one with the smallest distance
We don't have the space. The other fields on "struct commit" are already
used (indegree is used for topo sorting etc), and while we could make the
pointer itself point to a more complex structure rather than the name (one
that contains counts and possibly multiple names), that would now mean
that we'd have to make another allocation for each commit.
And that's very much against the whole point of the 'source' decoration.
It was designed to be basically zero-cost.
I could imagine doing it as not a single string: you could make it be a
pointer to a list of (alphabetically sorted) strings, and then you don't
have to make an allocation for each commit, you'd only need to do
something like
void add_source(struct commit *commit, struct strin_list *list)
{
struct string_list *old = commit->util;
if (!old) {
commit->util = list;
return;
}
if (old == list)
return;
.. do a union sort of 'old'/'list' ..
}
and I think it would be a stable algorithm (ie we'd share all normal
cases, and only have to allocate new lists in the relatively rare cases of
graphs joining), and that would be acceptable.
But the "counter" thing would not work. Not because it's expensive to
count (it's not - you just increment the counter every time you go to a
parent, and then if the parent already has a ->util entry, you replace it
if the new one has a smaller count), but because it's just expensive to do
another allocation for each commit.
(Of course, "expense" is relative. Maybe another allocation is ok, since
it would only trigger with --source.)
Linus
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
