Re: [RFC PATCH] unpack-trees: add check_worktree flag to enable dry-run functionality

From: Junio C Hamano <gitster@pobox.com>
Date: May 19, 2011, 18:14 UTC
Message-ID: <7vei3u7e25.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4DD557AF.4030700@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

Show 9 quoted lines

>>> +	if (opts.update && dry_run)
>>> +		opts.update = 0;
>> 
>> ... this hunk must go, right?
>
> But this is the "don't update the work tree when -n is used together
> with -u" part, so it is needed, no? With this patch applied first and
> opts.check_worktree set to 1 inside that if() added there all tests
> succeed.

I would say the natural way to do your "dry-run" would be to change the inner guts of unpack_trees() codepath that currently does

	if (opts.update) {
		if (do something to the work tree and get non-zero on failure)
			die("... cannot update '%s'", path);
	}

with your "-n" work to

	if (opts.update) {
        	if (opts.dry_run) {
			if (would the work tree operation fail?)
				say("... update would fail because ... '%s'", path);
		} else {
			if (do something to the work tree and get non-zero on failure)
				die("... cannot update '%s'", path);
		}
	}

and that was why I thought you would want to keep the original value of opts.update. I wouldn't think of a good way to make the code that kicks in when both update and dry_run are set if you clear update that early in the codepath.

Of course you could implement a roundabout logic that says something like

	if (!update && !pretending_to_update)
        	return 0; /* we are not updating */

	if (update)
        	do work tree operations;
	else if (pretending_to_update)
        	check if work tree operations would fail;
	else
		do nothing;

But I think that is going backwards. The point of "-n" == dry-run is to cover all cases, and your initial round was only covering "no -u" case and in this round we are trying to also cover "-u" case. With the approach to add "check-worktree", if we have the third mode of operation (the first two being the existing "without -u" and "with -u"), would you add yet another "check-distim"?

Wouldn't the interaction with these true "operation modes" and "dry run" be a lot simpler to read and implement that way? In other words, which one do you find easier between these two to follow?

	if (opts.update) {
		if (opts.dry-run)
                	check if we can update but do not update in real;
		else
                	do update and die if we can't;
	} else if (opts.distim) {
		if (opts.dry-run)
                	check if we can distim but do not actually distim it;
		else
                	do distim and die if we cannot;
	} ...

vs

	if (!opts.update && !opts.distim && !opts.check_worktree && !opts.check_distim)
		return;

	if (opts.update || opts.check_worktree) {
		if (opts.check_worktree)
                	check if we can update but do not update in real;
		else
                	do update and die if we can't;
	} else if (opts.distim || opts.check_distim) {
		if (opts.check_distim)
                	check if we can distim but do not distim in real;
		else
                	do distim and die if we can't;
        }