darcs

Patch 2270 Re: darcs patch: clarify help for --hash, --from-hash and... (and 2

Title Re: darcs patch: clarify help for --hash, --from-hash and... (and 2
Superseder Nosy List gpiero
Related Issues
Status in-discussion Assigned To
Milestone

Created on 2022-12-16.18:33:06 by gpiero, last changed 2023-05-28.09:15:42 by gpiero.

Files
File name Status Uploaded Type Edit Remove
accept-issue2695_-_log-__hash-x_-is-not-equivalent-to-_log-__matches-_hash-x__.dpatch gpiero, 2023-01-30.17:46:39 text/plain
clarify-help-for-__hash_-__from_hash-and-__to_hash-options.dpatch dead gpiero, 2022-12-16.18:33:05 application/x-darcs-patch
clarify-help-for-__hash_-__from_hash-and-__to_hash-options.dpatch dead gpiero, 2022-12-16.18:41:22 text/plain
patch-preview.txt dead gpiero, 2022-12-16.18:33:05 text/x-darcs-patch
See mailing list archives for discussion on individual patches.
Messages
msg23051 (view) Author: gpiero Date: 2022-12-16.18:33:05
Some very old (almost 6-year old) patches that I've just rebased against
screened. Should look at my todo list more often.

Not sure if the help text makes sense in English, looking for your judgement.

The rationale for 9b86a4f2b022bf1387d4d2d5c6c1a54abf7aa536 is that currently:

$ darcs log -h 5d6 --count
1
$ darcs log --matches 'hash 5d6' --count
5

While I don't really expect anyone to select a bunch of patches based on a
common hash prefix, this is for coherence and for honoring the contract with
the user.

3 patches for repository http://darcs.net/screened:

patch d682cdb483060d25b6921c75360c53cefe52c6eb
Author: Gian Piero Carrubba <gpiero@rm-rf.it>
Date:   Fri Dec 16 18:47:02 CET 2022
  * clarify help for --hash, --from-hash and --to-hash options

patch edabbbd32a9cd4091e4dff2f4f909f15c1446d52
Author: Gian Piero Carrubba <gpiero@rm-rf.it>
Date:   Fri Dec 16 18:59:36 CET 2022
  * mention --from-hash and --to-hash in the output of `darcs help patterns`

patch 9b86a4f2b022bf1387d4d2d5c6c1a54abf7aa536
Author: Gian Piero Carrubba <gpiero@rm-rf.it>
Date:   Fri Dec 16 19:07:43 CET 2022
  * add --hashes option and use it instead of --hash where appropriate
  This way, -h/--hash/--hashes really become aliases for
  --match/--matches 'hash ...', as suggested by `darcs help patterns`.
Attachments
msg23052 (view) Author: gpiero Date: 2022-12-16.18:41:23
mmpf, really need to check I've recorded all the changes before sending 
them...
Attachments
msg23071 (view) Author: ganesh Date: 2023-01-01.02:55:03
The changes make sense to me.

On the English, when you say "a patch whose hash prefix matches HASH" I 
guess this is meant to be parsed as "a patch whose hash (prefix matches) 
HASH"?

The tricky bit is that it probably more naturally parses as "a patch 
whose (hash prefix) matches HASH" which sort of implies the existence of 
a unique hash prefix.

I wonder if renaming the meta-variable and going for something like "a 
patch whose hash starts with HASHPREFIX" would work better?
msg23080 (view) Author: bfrk Date: 2023-01-14.19:22:56
I agree that the last wording proposed by Ganesh is the best option 
here.

Apart from that I am fine with the 3 patches. The code changes look 
reasonable to me. I would feel safer with a test or two, and would 
like to encourage you to add one, but this is not a requirement. 
(That would be hypocritical, given that we don't have any systematic 
tests for matching options yet.)
msg23084 (view) Author: gpiero Date: 2023-01-30.17:46:52
Updated bundle (with test).
Attachments
msg23093 (view) Author: bfrk Date: 2023-02-16.09:21:01
I see no tests being added. Have you forgot to add them? Otherwise 
fine.
msg23094 (view) Author: bfrk Date: 2023-02-16.09:22:56
Can I mark issue2695 as resolved when this is pushed?
msg23105 (view) Author: bfrk Date: 2023-02-19.08:45:17
Pushed latest bundle to screened.
msg23115 (view) Author: bfrk Date: 2023-02-20.21:55:09
It does indeed resolve issue2695. Anyway, accepted.
msg23228 (view) Author: gpiero Date: 2023-03-29.15:49:12
* [Thu, Feb 16, 2023 at 09:21:06AM +0000] Ben Franksen:
>I see no tests being added. Have you forgot to add them? Otherwise
>fine.

It seems that the wrong bundle has been screened. More precisely, [0] 
(wrong one) has been screened instead of [1] (right one).
In another thread Ganesh suggested to mark superseded bundles as dead.  
Maybe deleting them causes less confusion (at the cost of losing some 
history)?

Regarding these patches, can you check if [1] is OK?
Once that's done, let me know if you prefer to rollback the old ones and 
apply the new ones (if that can be done cleanly), apply a fix-it-all 
patch or simply let the things as they are.

* [Thu, Feb 16, 2023 at 09:22:57AM +0000] Ben Franksen:
>Can I mark issue2695 as resolved when this is pushed?

I thought there were a hook that automatically did this when a patch 
name loosely matched 'resolve issuexxx', am I wrong?

Ciao,
Gian Piero.

[0] 
http://bugs.darcs.net/file9566/clarify-help-for-__hash_-__from_hash-and-__to_hash-options.dpatch

[1] 
http://bugs.darcs.net/file9575/accept-issue2695_-_log-__hash-x_-is-not-equivalent-to-_log-__matches-_hash-x__.dpatch
msg23229 (view) Author: bfrk Date: 2023-03-30.06:39:56
OMG, sorry for the mess! I am not sure how best to fix this. Ganesh do you 
have a suggestion?

I personally prefer to obliterate the old bundle in screened. The only 
persons affected are you, me, and Ganesh. Among us three we should be able to 
manage. There may be lurkers but if they don't do any development of their 
own they should be fine.

> I thought there were a hook that automatically did this when a patch
> name loosely matched 'resolve issuexxx', am I wrong?

It seems this automation was lost when we moved things to a new server. I 
tried to restore it from the parts of the old machine that we archived but 
could not find out how it was done. There are no hooks in the old repos and 
nothing related to this kind of functionality in the old roundup 
configuration. It is possible that it was instead triggered by the email 
processing machinery but I have no idea how that works and where it is 
configured.
msg23230 (view) Author: bfrk Date: 2023-03-30.09:07:16
Sorry for bringing it up so late in the process, but after thinking again about this 
change I have serious doubts.

Yes, it makes the UI more consistent, which is a good thing per se. However, from a 
practical perspective, I'd really prefer it if darcs would fail if the given hash 
prefix is not unique (which is how it works in git, IIRC). When you give darcs a 
hash, you are implicitly saying "I want precisely this (unique) patch". Why else 
would you bother to use a hash match? This applies to the singular version of the 
option; the plural version (--hashes) is useless IMO and should be removed. For match 
expressions (the --*match* options) I would also want uniqueness for every 'hash' 
term; the result can still be multiple patches e.g. if you use 'hash xx or hash yy'.

This is related to my long-standing issue with tag matches: like for hashes, when you 
match on a tag you usually want a unique match (when using the singular form of the 
option). There may be the occasional exception but I'd rather have to use a work-
around in these few cases than the other way around.

One idea to solve the UI problem is to provide a --unique option, which would qualify 
any match expression to express that the match must be unique or else fail. But --
unique doesn't make much sense for many of the allowed matches (why would you ever 
want to get a unique match for date, author, or long comment?). It is also very 
inefficient for anything that needs to access patch content i.e. hunk and touch. In 
practice the only matches where you may want unique matches are: tag, hash, and name. 
We could limit --unique to these three match types and fail if you try to use it with 
any other match expression.

In the end I think this is too much complication for too little gain. We should 
simply bite the bullet and require (and check) that (singular) 'tag' and 'hash' 
matches are unique (and clearly document that). This is certainly the most useful 
behavior in practice; and (IME) it is also the behavior naive users expect. Strictly 
speaking this breaks compatibility, but I am almost sure nobody relies on the current 
behavior for correctness.

(Side note: implementing a uniqueness queck probably requires some refactoring of the 
matching machinery. Also, without some sort of index it will be linear in the number 
of patches in the repo, which may turn out to be prohibitively expensive.)
msg23231 (view) Author: ganesh Date: 2023-03-30.13:19:42
> I personally prefer to obliterate the old bundle in screened. The
> only persons affected are you, me, and Ganesh. Among us three we
> should be able to manage. There may be lurkers but if they don't
> do any development of their own they should be fine.

I think there's also this mirror:

https://hub.darcs.net/darcs/darcs-screened

I'm fine with obliterating if that's what you prefer.
msg23233 (view) Author: bfrk Date: 2023-03-31.11:11:26
Marking as "in-discussion". I will obliterate the patches from 
screened and the hub.darcs mirror (and also check reviewed just to 
be sure).

@gpiero I would love to get your feedback regarding my reservations 
and the counter-proposal.
msg23234 (view) Author: bfrk Date: 2023-03-31.11:31:42
@ganesh I seem to be unable to obliterate from 
https://hub.darcs.net/darcs/darcs-screened. I tried

> ssh bf@hub.darcs.net obliterate darcs/darcs-screened
invalid repository name
> ssh darcs@hub.darcs.net obliterate darcs-screened
darcs@hub.darcs.net: Permission denied (publickey).

and as darcs-unstable@darcs.net:
> ssh darcs@hub.darcs.net obliterate darcs-screened
Unable to negotiate with 173.255.254.113 port 22: no matching cipher 
found. Their offer: aes256-cbc,aes192-cbc,aes128-cbc
msg23278 (view) Author: gpiero Date: 2023-05-28.09:15:41
* [Thu, Mar 30, 2023 at 09:07:16AM +0000] Ben Franksen:
>In the end I think this is too much complication for too little gain. 
>We should simply bite the bullet and require (and check) that 
>(singular) 'tag' and 'hash' matches are unique (and clearly document 
>that). This is certainly the most useful behavior in practice; and 
>(IME) it is also the behavior naive users expect. Strictly speaking 
>this breaks compatibility, but I am almost sure nobody relies on the 
>current behavior for correctness.

As for 'hash', I think it's the best option, especially if darcs errors 
out outputting a list of possible matches, like git does.
Regarding 'tag', I've have some instinctive doubts. I use darcs for 
personal software and I don't have a need for tags, so I don't have a 
strong opinion on this. But, for example, the use documented in 
http://bugs.darcs.net/issue2702 seems legit. OTOH, I cannot think of a 
single case in which a pattern match on the hash can be legit.

>(Side note: implementing a uniqueness queck probably requires some 
>refactoring of the matching machinery. Also, without some sort of index 
>it will be linear in the number of patches in the repo, which may turn 
>out to be prohibitively expensive.)

I've had a cursory look and I have no idea how and where to hook it.  
So, as usual for darcs things: yes, Ben, thank you. :)
History
Date User Action Args
2022-12-16 18:33:06gpierocreate
2022-12-16 18:41:27gpierosetfiles: + clarify-help-for-__hash_-__from_hash-and-__to_hash-options.dpatch
messages: + msg23052
title: clarify help for --hash, --from-hash and... (and 2 more) -> Re: darcs patch: clarify help for --hash, --from-hash and... (and 2
2023-01-01 02:55:04ganeshsetstatus: needs-screening -> review-in-progress
messages: + msg23071
2023-01-14 19:22:57bfrksetmessages: + msg23080
2023-01-30 17:46:54gpierosetfiles: + accept-issue2695_-_log-__hash-x_-is-not-equivalent-to-_log-__matches-_hash-x__.dpatch
messages: + msg23084
2023-02-16 09:21:06bfrksetmessages: + msg23093
2023-02-16 09:22:57bfrksetmessages: + msg23094
2023-02-19 08:45:19bfrksetmessages: + msg23105
title: Re: darcs patch: clarify help for --hash, --from-hash and... (and 2 -> clarify help for --hash, --from-hash and... (and 2 more)
2023-02-20 21:55:10bfrksetstatus: review-in-progress -> accepted-pending-tests
messages: + msg23115
2023-03-29 15:49:13gpierosetmessages: + msg23228
title: clarify help for --hash, --from-hash and... (and 2 more) -> Re: darcs patch: clarify help for --hash, --from-hash and... (and 2
2023-03-30 06:40:06bfrksetmessages: + msg23229
2023-03-30 09:07:16bfrksetmessages: + msg23230
2023-03-30 13:19:43ganeshsetmessages: + msg23231
2023-03-31 11:11:26bfrksetstatus: accepted-pending-tests -> in-discussion
messages: + msg23233
2023-03-31 11:31:43bfrksetmessages: + msg23234
2023-05-28 09:15:42gpierosetmessages: + msg23278