Re: [RFC/PATCH] rev-list: new --cherry-pick=loose option

[Jay Soffian <jaysoffian@gmail.com>]

On Mon, Feb 21, 2011 at 7:16 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>> Thoughts?
>
> I am personally not very interested in this particular "author, timestamp,
> title and nothing else" implementation, as that is probably too loose (in
> many projects, title by itself is not descriptive enough) to be safe.

Well, right, it's not title by itself, it's title + authorship. I am
scratching my head at a project that would have commits identical in
author, timestamp, and title.

> Also people would probably want other loose modes with varying degree

Initially I thought this too and was going to let it take pretty-print
specifiers (--cherry-pick=format:...), but I can't see which other
metadata would be useful. Maybe the commit body, but if there were
conflicts, that's something that would have very likely been changed
to add a Conflicts: section.

> (e.g. throwing in the list of touched paths to your mix might make it a
> bit safer), so "loose" feels a bit too broad a word to give to this
> particular implementation (iow, it does not say in what way it is loose).

Throwing in touched paths does make it safer, but for my use case is
too strict for some commits (I sometimes have to account for file
renames when I cherry-pick).

>> @@ -65,8 +79,13 @@ static struct patch_id *add_commit(struct commit *commit,
>>       unsigned char sha1[20];
>>       int pos;
>>
>> -     if (commit_patch_id(commit, &ids->diffopts, sha1))
>> -             return NULL;
>> +     if (ids->loose) {
>> +             if (commit_patch_id_loose(commit, sha1))
>> +                     return NULL;
>> +     } else {
>> +             if (commit_patch_id(commit, &ids->diffopts, sha1))
>> +                     return NULL;
>> +     }
>
> If the purpose of the patch is to stir the discussion, it is fine to have
> any crappy "here is a strawman" algorithm as an example of an alternative
> patch ID computation, but one thing it _should_ do well is to show where
> the necessary change should be hooked into, and I think the above "if"
> statement is placed in a wrong function.  If you change commit_patch_id()
> to take a pointer to the whole ids instead of just &ids->diffopts, it can
> decide how the "commit patch ID" is computed without affecting the
> callers, no?

Good idea.

> And then we could instead introduce patch-id-algo=<foo>, and instead of
> "loose" call this particular algorithm "authorship-subject" or something.
> Coming up with a pluggable interface to let the end user compute patch
> equivalence might be a plus.

I think it's premature to make it pluggable. I was hoping that with
this mode (loose) and the existing mode (strict), the extremes would
be covered. And then we'd wait and see what latent demand might exist
for other modes. :-)

I just don't feel like I know enough about what "the end user" wants
to come up with a reasonably general scheme.

(The best I can come up with that's pluggable and not horribly
expensive is something like open bi-directional pipes to an external
process that gets fed diffs and returns patch-ids.)

> Certain patch equivalence might not be easy to define by "do they hash to
> the same small value" but by "here are two patches---compare them and tell
> me if they are equivalent".  If we can update the code to support that
> kind of patch equivalence it would be great, but it is not within the
> reach/scope of this patch (not a complaint, but something you may want to
> tackle next).

More than I can bite off at the moment.

j.