Patch 1700 Consistent naming for working tree

Title Consistent naming for working tree
Superseder Nosy List raichoo
Related Issues
Status accepted Assigned To

Created on 2018-06-28.15:49:37 by raichoo, last changed 2018-09-14.08:06:56 by ganesh.

File name Status Uploaded Type Edit Remove
consistent-naming-for-_working-tree_.dpatch raichoo, 2018-06-28.16:11:55 application/octet-stream
consistent-naming-for-_working-tree_.dpatch raichoo, 2018-07-05.09:51:38 application/octet-stream
consistent-naming-for-_working-tree_.dpatch raichoo, 2018-07-16.07:52:12 application/octet-stream
consistent-naming-for-_working-tree_.dpatch raichoo, 2018-07-16.16:38:30 application/octet-stream
consistent-naming-for-_working-tree_.dpatch ganesh, 2018-08-28.06:39:18 application/x-darcs-patch
patch-preview.txt ganesh, 2018-08-28.06:39:18 text/x-darcs-patch
unnamed ganesh, 2018-08-28.06:39:18 text/plain
See mailing list archives for discussion on individual patches.
msg20176 (view) Author: raichoo Date: 2018-06-28.15:49:37
While writing a book on darcs I found that there are some naming
inconsistencies that might be confusing to newcomers.
msg20181 (view) Author: bf Date: 2018-07-04.13:20:55
This changes "working directory" to "working tree" even where the
meaning is "current working directory" i.e. wrt file system semantics.
This is not at all the same as what we call the "working tree".

I am all for consistent naming but in this case all the places must be
reviewed manually to change only those where the meaning is actually
"working tree" in the sense of "the directories and files that the user
edits" as opposed to e.g. the "pristine tree" which is the recorded
state of the repo.
msg20182 (view) Author: raichoo Date: 2018-07-04.14:19:22
Okay, I will take a closer look at this. I should probably rename most
occurrences of "working direction" to "working tree" in the user facing
code though, maybe I went a bit over board with renaming everything. Do
you have a specific example where my patch breaks semantics? Any thoughts?
msg20183 (view) Author: raichoo Date: 2018-07-05.09:51:38
Hey, I've reviewed the code to the best of my knowledge and found a
couple of the semantic issues you were talking about. I've updated the
patch, feedback greatly appreciated :)
msg20184 (view) Author: bf Date: 2018-07-12.19:02:59
Thanks! I am back from holidays (for now) and will take a look at your
new patch.
msg20187 (view) Author: bf Date: 2018-07-14.15:56:17
There remain issues with the patch.

In D.R.Job the meaning is 'working directory' as stated, not 'working
tree'. Also in D.UI.Completion and D.Util.Tree.Monad and D.Util.Path and
in tests/mv.sh. In D.Util.Index only the first change in the module
header comment is correct, for the other two the meaning is 'directory',
not 'tree'.

In D.R.State, one of the changed comments no longer makes sense:
'Obtains a Tree corresponding to the complete working tree...'; this
should be shortened to 'Obtains the complete working tree...'. Also on
line 313 'working dir' should also be replaced with 'working tree'.

In D.R.Merge the debug message is not very helpful and should be
re-written to better conform to what is happening; but we can do that in
a separate patch.
msg20188 (view) Author: raichoo Date: 2018-07-16.07:52:12
Thanks for the feedback. I've amended the patch according to your comment.
msg20189 (view) Author: bf Date: 2018-07-16.16:23:05
Thanks, much better. Only two small nitpicks remain:

In Darcs.UI.Completetion you went a bit too far with re-replacing: There
was a 'working tree' in a comment that you, I think accidentally,
changed to 'working directory'.

In Darcs.Repository.Hashed the change on line 444 is wrong; look at the
original sentence: "Read a 'PatchSet' from the repository (assumed to be
located at the current working directory)"; replacing directory with
tree here sounds weird and is semantically wrong.

BTW, the easiest way to fix the patch is to amend --unrecord the two
changes, then revert them, then re-send.

If you fix these, your patch is ready for screened. Thanks again for the
contribution and looking forward to more of the same or similar. I am
constantly trying to move the code base toward more consistency and
every contribution toward that goal is worth the effort.

I may choose to rebase your patch on top of my latest refactorings at
https://hub.darcs.net/bf/darcs-bf-latest as there are conflicts which
are easier to resolve this way than by rebasing all my 60 or so patches.
This is /not/ the usual procedure and I am myself to blame for the
situation because I did not manage to properly send patch bundles and
screen them yet. I hope this won't bother you. (I will take care to not
change any of the meta-data of your patch.)
msg20190 (view) Author: raichoo Date: 2018-07-16.16:38:30
I rather have someone familiar with the code nitpicking my patches than
contributing something that doesn't improve darcs so your feedback is
highly appreciated :). Let's see if this is better. Thanks again, for
taking the time to review this.
msg20275 (view) Author: ganesh Date: 2018-08-28.06:39:18
Sending the latest version of this patch that's in screened to the tracker
1 patch for repository /home/ganesh/darcs/screened-temp:

patch 930d57af79a64224ba7cf6a09079a7e069c39ed4
Author: raichoo@googlemail.com
Date:   Mon Jul 16 17:39:39 BST 2018
  * consistent naming for "working tree"
msg20276 (view) Author: bf Date: 2018-08-28.07:13:51
Oops, thanks for catching that ommision (I had to rebase it slightly).

Also, this patch is actually fully reviewed, but (due to the rebase) it
depends on not-yet-accepted patches I sent. Should I set the status to
accepted-pending-tests? Who will push it to reviewed once its
dependencies are?
msg20279 (view) Author: ganesh Date: 2018-08-28.16:25:58
I use accepted-pending-tests for that situation, and then try to 
remember to go back and push it when it's possible.

(I have a tool that scrapes the patch tracker and the repos that helps 
me keep track of this)
msg20284 (view) Author: bf Date: 2018-08-28.18:47:27
Okay, doing that now.
Date User Action Args
2018-06-28 15:49:38raichoocreate
2018-06-28 16:11:55raichoosetfiles: + consistent-naming-for-_working-tree_.dpatch
2018-06-28 16:12:20raichoosetfiles: - consistent-naming-for-_working-tree_.dpatch
2018-07-04 13:20:56bfsetmessages: + msg20181
2018-07-04 14:19:22raichoosetmessages: + msg20182
2018-07-05 09:51:38raichoosetfiles: + consistent-naming-for-_working-tree_.dpatch
messages: + msg20183
2018-07-12 19:03:05bfsetmessages: + msg20184
2018-07-14 15:56:17bfsetmessages: + msg20187
2018-07-16 07:52:12raichoosetfiles: + consistent-naming-for-_working-tree_.dpatch
messages: + msg20188
2018-07-16 16:23:06bfsetmessages: + msg20189
2018-07-16 16:38:31raichoosetfiles: + consistent-naming-for-_working-tree_.dpatch
messages: + msg20190
2018-08-26 17:31:06bfsetstatus: needs-screening -> needs-review
2018-08-28 06:39:18ganeshsetfiles: + patch-preview.txt, consistent-naming-for-_working-tree_.dpatch, unnamed
messages: + msg20275
2018-08-28 07:13:51bfsetmessages: + msg20276
2018-08-28 16:25:58ganeshsetmessages: + msg20279
2018-08-28 18:47:27bfsetstatus: needs-review -> accepted-pending-tests
messages: + msg20284
2018-09-14 08:06:56ganeshsetstatus: accepted-pending-tests -> accepted