Re: [PATCH/RFCv3 2/2] receive-pack: don't pass non-existent refs to post-{receive,update} hooks in push deletions

Pang Yan Han [off-list ref] writes:

+/* For invalid refs */
+static struct command **invalid_delete;
+static size_t invalid_delete_nr;
+static size_t invalid_delete_alloc;

Do you have to have these separately only to keep track of the corner case
errors?  I would have expected that it would be more natural to mark them
by adding a single bitfield to "struct command".

This is not an "invalid" delete but deleting a non-existing ref.  Perhaps
you would want to move the warning and optionally reword it as well, e.g.

	if (!parse_object(old_sha1)) {
        	old_sha1 = NULL;
                if (ref_exists(name)) {
			rp_warning("Allowing deletion of corrupt ref.");
		} else {
			rp_warning("Deleting a ref that does not exist.");
			cmd->did_not_exist = 1;
		}
		...

On Wed, Sep 28, 2011 at 03:37:32PM -0700, Junio C Hamano wrote:

Pang Yan Han [off-list ref] writes:

quoted

+/* For invalid refs */
+static struct command **invalid_delete;
+static size_t invalid_delete_nr;
+static size_t invalid_delete_alloc;

Do you have to have these separately only to keep track of the corner case
errors?  I would have expected that it would be more natural to mark them
by adding a single bitfield to "struct command".

Yes they are purely for keeping track of deleting non-existent refs.
Ok I will add the bitfield to struct command.

quoted

@@ -447,6 +467,8 @@ static const char *update(struct command *cmd)
 		if (!parse_object(old_sha1)) {
 			rp_warning("Allowing deletion of corrupt ref.");
 			old_sha1 = NULL;
+			if (!ref_exists((char *) name))
+				invalid_delete_append(cmd);

This is not an "invalid" delete but deleting a non-existing ref.  Perhaps
you would want to move the warning and optionally reword it as well, e.g.

	if (!parse_object(old_sha1)) {
        	old_sha1 = NULL;
                if (ref_exists(name)) {
			rp_warning("Allowing deletion of corrupt ref.");
		} else {
			rp_warning("Deleting a ref that does not exist.");
			cmd->did_not_exist = 1;
		}
		...

Sure thing.

I am unable to reply this until much later, but are the tests in this patch ok?
It's the first time I'm writing test cases for git.

Thanks for the feedback!