[darcs-devel] darcs patch

[Karel Gardas]

David,

first of all, thanks for your kind review. This patch was intended just
for start of design discussion. It seems you have not tried it directly,
so I will at the first describe intended implementation and then have some
notes below.
This patch changes patch ids as represented in inventory file and patch
file names in patches directory. Example of patch which is created by
using this functionality is:

file name:
20041129233520-0cd4196f-835b-4c3c-a54f-63e9604a5ec4-fb047-9c569495995ed25c450e05bbe01af2d9ed7e01ce.gz
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
patch id:
[hhh
Karel Gardas <kgardas at objectsecurity.com>**20041129233520-0cd4196f-835b-4c3c-a54f-63e9604a5ec4]
                                                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

UUID generated parts are marked by `^' characters.

The main question is if you agree with that design decission and if you
think this is the right solution of PR#5. If you agree, then I can improve
this patch using your ideas. W.r.t. compatibility. Whole implementation is
backward, but not forward compatibile, this means that the new darcs is
able to work (should be) with patches generated by old darcs and also by
this new darcs, but old darcs is not able to work with patches generated
by new darcs (i.e. forward compatibility is not preserved). It would be
great if we can discuss such compatibility issues before going to improve
this support...

But now, some comments below.

On Tue, 30 Nov 2004, David Roundy wrote:

> On Tue, Nov 30, 2004 at 09:17:33AM +0100, Karel Gardas wrote:
> > +ifeq ($(HAVE_UUIDGEN),True)
> > +GHCFLAGS += -DHAVE_UUIDGEN
>
> I'd rather you define a variable in Autoconf.lhs and use ACSUBST in the
> configure script.  This has the advantage of causing both versions to be
> compiled, so errors are easier to find.  The compiler should be smart
> enough that only one side of an inlined False comparison is actually
> generated.  If the code wouldn't even compile in the absence of uuidgen,
> that would be a different case.

OK, I will investigate this.

> > hunk ./PatchInfo.lhs 131
> > - -make_filename (PatchInfo dps nps aps lps inv) =
> > - -    cleanDate d++"-"++sha1_a++"-"++sha1PS sha1_me++".gz"
> > +make_filename (PatchInfo dps ups nps aps lps inv) =
> > +    cleanDate d ++u++"-"++sha1_a++"-"++sha1PS sha1_me++".gz"
>
> I'm concerned about compatibility here with existing darcs.  I'm thinking
> you need to change
>
>               sha1_me = concatPS [nps, aps, dps, concatPS lps, b2ps inv]
>
> to
>               sha1_me = concatPS [nps, aps, dps, packString "-",ups,
>                                   concatPS lps, b2ps inv]
>
> and can't include the ++u after cleanDate d.

Hmm, that was intended design decision to include uuid in file name.

> The output of make_filename needs to be the same as older versions of darcs
> would give, when they parse this PatchInfo, which brings up another
> question...

It seems that I have understand PR#5 in a wrong way, but I'v thought that
if we are going to increase patch ids space, then we also need to increase
file name space, at least that was my idea. Thinking about it again, it
seems file names space is already increased by using sha1 added to it,
while patch ids does not use sha1...

> > hunk ./PatchInfo.lhs 198
> > - -showPatchInfo (PatchInfo ct name author log inv) =
> > +showPatchInfo (PatchInfo ct uuid name author log inv) =
> > hunk ./PatchInfo.lhs 200
> > - -  unpackPS author ++ inverted ++ unpackPS ct ++
> > +  unpackPS author ++ inverted ++ unpackPS ct ++ unpackPS uuid ++
>
> Is the uuid only digits? It looks (from below) like there needs to be a '-'
> between the uuid and the ct.

While generating uuid, I'm prepending it with `-'. This is done for remove
a need to change everywhere if uuid generated is empty string or not and
then use nothing or `-' for concatenating it with some other string...

> I'm concerned as to how older darcs will
> parse this PatchInfo.  Presumably, it would just think that the uuid is
> part of the date.  But then what will happen when it runs cleanDate on
> that? (I don't know the answers here, am just asking the questions... but
> it looks to me like this would be buggy.)

Old darcs does not parse new patch info, you are right, it chokes on date
parsing claiming `-' is wrong character...

> BTW, there may not *be* a clean solution, depending on the behavior of
> cleanDate.  But if that is the case, we'll have to plan a transition, which
> most likely would have to wait for darcs 1.2 or 2.0.  A brief look at
> parseDate suggests that this may be a problem.  In which case a patch to
> allow parseDate to accept an ISO date followed by garbage (e.g. a uuid)
> could be included now, followed by a patch such as yours later.  But I
> *really* dislike breaking backwards compatibility, so later could mean much
> later.

IMHO backward compatibility is preserved, forward is not.

> Perhaps (for example) at the same time as we have a flag day for a
> new merger algorithm.

Yes, you are probably right, but at least we can at least try to make
things forward compatibile at least in this date parsing domain...

> > hunk ./Record.lhs 270
> > +
> > +get_uuid :: [DarcsFlag] -> String
> > +get_uuid _ = genUuid
> > +
>
> I don't like this function.  It seems better to just use genUuid in the
> first place.  Adding unused arguments seems a recipe for confusion.

OK.

> > +--
> > +-- Generates UUID by using uuidgen command-line utility
> > +--
> > +genUuid :: String
> > +genUuid = "-" ++ uuid
> > +    where uuid = unsafePerformIO $ withTemp $ \uuidgen_output -> do
> > +	  exec "uuidgen" [] "/dev/null" uuidgen_output
> > +	  to <- readBinFile uuidgen_output
> > +	  return (trans to)
>
> Shouldn't this be getnUuid :: IO String?
>

:-) I've tried but as a pure Haskell newbie, I haven't know how to declare
and implement such function for a case when HAVE_UUIDGEN is not defined
and when I just need to return empty string. Yes, I still need to learn
something about Haskell IO...

> Then you won't need unsafePerformIO, and it'll also be correct, since I
> assume that it is intended to give a different answer each time it's
> invoked? Basically, this code scares me because I'm not sure how it'll
> behave, I think it may behave differently depending on whether or not it is
> inlined.
>
> This code also needs some error checking.  Just because uuidgen existed
> when we ran configure doesn't mean that it exists when darcs runs later.
> We should catch exceptions and do something reasonable, like either
> returning "", or perhaps failing with an informative error message, or
> resorting to a cruder way of getting some randomness.

You are right, it needs quite a load of fixes before going to production
environment!

> > [resolve conflicts after pull
> > Karel Gardas <kgardas at objectsecurity.com>**20041130074548] {
>
> Since I'm not accepting this patch yet, you could unrecord the conflict
> resolution patch, and then amend-record the original patch... basically
> coalescing these patches into one, which'll make it a bit easier to read.

Err, ok, although as a long term Arch users I'm not that used to throw
patches away :-)

Thanks!

Karel
--
Karel Gardas                  kgardas at objectsecurity.com
ObjectSecurity Ltd.           http://www.objectsecurity.com