Re: [PATCH 2/2] Add a 'source' decorator for commits

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.

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