[darcs-users] darcs patch: resolve issue628: reuse the long comment code from Dar...

Eric Kow kowey at darcs.net
Fri Oct 24 12:45:25 UTC 2008


Almost there...

On Fri, Oct 24, 2008 at 13:58:59 +0200, Christian Kellermann wrote:
> > Unless I am mistaken, by using Darcs.Record.get_log, we now no longer
> > ask "What is the version name?" but "What is the patch name?"
> 
> That's right, at the moment I don't see that as a problem since a
> tag is nothing more than a patch right?

It might confuse some people, but I don't see it as grounds for
rejecting the patch (besides, somebody else can follow up by
parameterising the the get_log function, although unfortunately
that means more complicated code... which could just mean we need
to rewrite get_log)

> > This is going to sound like the opposite of what I suggested last
> > time, is there a reason why get_name_log is in a where block and not a
> > top-level function?  It's not always easy to get a feel for when to do
> > one or the other, so it'll be good see your thoughts on it.  I was
> > mainly asking because not moving it may allow you make an even more
> > minimal patch, but you have had some design-thinking in there...

> Well this is mainly based on David's comment that he would not make
> functions available that are used only once.  I am not sure whether
> I understand your argument about making an even shorter patch when
> moving it...

I think I was just slightly confused.  I was saying that if you had just
replaced get_patchname by an equivalent get_name_log in the exact same
spot in the file, the patch might be even smaller.  But it turns out
that it won't be (except maybe that it would look nicer in a graphical
diff program, e.g. with darcs diff --diff-command="meld %1 %2" --last=1)

It's fine either way.

> > Note that this represents a change in behaviour.  Now you can no longer
> > name your tags TAG something (which would be silly to do anyway on the
> > other hand).
> 
> Could you do that before? Then I don't get the check in get_log at Record.lhs.

I think so, and the reason I say this is that all of the code in
Darcs.Commands.Tag just simply does ("TAG " ++).

I think the reason may be that darcs actually uses the fact that the
patch name starts with "TAG " to determine if a patch is a tag or not.
So we need to protect against users accidentally naming their patches
'TAG ' (for example, I used to work on a natural language generator
using the Tree Adjoining Grammar formalism... and could easily have
given a patch a name like 'TAG substitution operation')

This check isn't quite so critical to the darcs tag command, so we
should probably keep it simple and just do ("TAG " ++)

> > So if the user specified a patch name with -m, we ignore the patch
> > name supplied on the command line args... fair enough, I guess :-)
>  
> That's how its been before as well if IIRC.

Ok.

Oh and one last remark: the tidying up of the inner do block
(tentativelyAddPatch etc) is likely a good thing, but it should probably
go in a separate patch since it's not related.  Having minimal patches
is nice because then people can review them very easily.  None of these
are hard-and-fast rules, just sort of us trying to help each other out.

Thanks!

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : http://lists.osuosl.org/pipermail/darcs-users/attachments/20081024/3ff387af/attachment.pgp 


More information about the darcs-users mailing list