Jens Lehmann <Jens.Lehmann@xxxxxx> writes:
>>> + 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;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html