[RFC] fast-import: note deletion command

Hi,

fast-import is able to write notes provided a command N <note_content_data_specification> <commit_specification>

There is no documented command to delete a note. It can't be easily replaced with Delete command on notes tree because of notes fanout.

But fast-import interprets 0{40} data_specification as a note deletion command. It is consistent with fast-import internals where null_sha1 is used for absent/unknown object. But is not consistent with file Modify command nor looks like a common convention in git ui.

So the questions are:
- should 0{40} not be treated as deletion toggle? Downside is that
it is used as one in t/t9301-fast-import-notes.sh.
- how should a documented way to delete notes look like?
ND commit_sha1 # One may think there are also NC, NR, NM
N delete commit_sha1
# Only :mark, full-40-byte-sha1 and 'inline' are allowed currently.
# So no clashes arise, but then one may also want M 'delete' path
# command to work too.

(+cc: Sverre for fast-import, Johan for notes, Thomas as a user of
 the note import feature, snipping less for their benefit)

Dmitry Ivankov wrote:

Right.

If I were doing it:

 - advertise "N 0{40} <commit>" as the historical way to delete a
   note, and make sure it keeps working

 - introduce 'ND' or 'notedelete <commit>' as a human-friendly
   synonym, keeping in mind that NC and NR could be introduced in the
   future if there is demand for them.

 - don't add any new magic "N <magic keyword> <commit>" commands,
   since it's hard to know when they will clash with some VCS's
   convention for object names.

I imagine there are plenty of other acceptable ways to accomplish these things, though. :) Thanks for looking into it.

Heya,

On Sun, Sep 18, 2011 at 22:35, Jonathan Nieder <jrnieder@gmail.com> wrote:

Is this perhaps a good moment to also think about branch deletion? That came up earlier as well, and thinking about that might give us some insights in how to deal with deletions uniformly.

On Mon, Sep 19, 2011 at 2:39 AM, Sverre Rabbelier <srabbelier@gmail.com> wrote:

Also maybe marks deletion, getting mark sha1, resetting a mark with explicit sha1. But most probably not tags deletion.

And going much further on commands, following look nice to have:
- 'ls' storing result to a mark (to allow us not to compute sha1/store
  object if we don't want to)
- marks namespaces (to keep ls mark separately, mark deletion will
  do too, if it's only a single temporary mark). Maybe like ::nr:mark.

Dmitry Ivankov wrote:

It might be a good temporal moment, but it's not quite the right thread for it. :) If someone wants to maintain a fast-import wishlist in a wiki somewhere, it sounds like it could be useful to some people (I prefer to read patches and stories about particular use cases, myself :)).

Regards, Jonathan

Sverre Rabbelier wrote:

Sorry, my earlier reply missed the point. To answer your question: Dmitry's RFC was about deleting individual notes rather than entire notes refs, so it is not too closely related to branch deletion.

A new "deleteref" command would be a very simple addition, and the UI seems pretty obvious. The main concern in adding such a thing is that it destroys history, while currently "git fast-import" without -f does not provide any commands that can do that. So presumably one would want to do one of the following:

 - only allow deletion of refs that were not present at the start of
   the import, or
 - add a command-line option to permit deletion of refs, so it doesn't
   happen by mistake when misparsing a stream.

It would also be interesting to see an example use (do you remember the earlier thread where it came up?) to make sure the UI fits it well. In most typical cases I can imagine, direct use of "git update-ref -d" would work better. In the case of remote helpers, IIRC there was already a need for the transport-helper to handle the final ref updates so "git fetch" can write a nice notice about them to the console.

Heya,

On Sun, Sep 18, 2011 at 23:18, Jonathan Nieder <jrnieder@gmail.com> wrote:

That book-keeping is trivial to do. The problem currently is that when you try to "git push origin :experimental-branch", there is no way for the transport-helper code to tell the helper to delete the ref. Something like 'deleteref' sounds sane I suppose, and I agree that there should be some sort of safety switch :). I think we need a new feature announcing that we require support for it, and then the importer can abort right away if the user doesn't want to have their refs deleted.

Sverre Rabbelier wrote:

Ah!  Thanks for explaining.

It also occurs to me that

	reset refs/heads/experimental-branch

	... rest of the fast-import stream comes here ...

could be used as an especially non-self-explanatory way to express this kind of thing. :) No idea whether that works already. A deleteref command and deleteref feature documented to be meant for this purpose sound handy to me.

By the way, what does the "export" command do in the following situation?

	git push origin something-big:master

Does it assume the remote-tracking branch for master reflects what's available on the other end and send a stream for origin/master..something-big, or does it send the entire history of something-big?

On Mon, Sep 19, 2011 at 3:49 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

     feature force
     reset refs/heads/experimental-branch
Will be close. It'll overwrite the branch if any commits are done to
it. But will
keep it as is in case it's not touched in the rest of the fast-import stream.

Heya,

On Sun, Sep 18, 2011 at 23:49, Jonathan Nieder <jrnieder@gmail.com> wrote:

We have a test case for this actually. It goes like this:

checking known breakage:
	(cd clone &&
	 git push origin new-name:new-refspec
	) &&
	compare_refs clone HEAD server refs/heads/new-refspec

Everything up-to-date fatal: Needed a single revision not ok 16 - push new branch with old:new refspec # TODO known breakage

In other words, we don't handle it at all. What we do handle a case similar to what you say, where we have already pushed part of the history of some-branch, and in that case we do indeed only push the needed objects.