Hi,
On Mon, Aug 8, 2016 at 3:32 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> * js/am-3-merge-recursive-direct (2016-08-01) 16 commits
> (merged to 'next' on 2016-08-05 at dc1c9bb)
> + merge-recursive: flush output buffer even when erroring out
> + merge_trees(): ensure that the callers release output buffer
> + merge-recursive: offer an option to retain the output in 'obuf'
> + merge-recursive: write the commit title in one go
> + merge-recursive: flush output buffer before printing error messages
> + am -3: use merge_recursive() directly again
> + merge-recursive: switch to returning errors instead of dying
> + merge-recursive: handle return values indicating errors
> + merge-recursive: allow write_tree_from_memory() to error out
> + merge-recursive: avoid returning a wholesale struct
> + merge_recursive: abort properly upon errors
> + prepare the builtins for a libified merge_recursive()
> + merge-recursive: clarify code in was_tracked()
> + die(_("BUG")): avoid translating bug messages
> + die("bug"): report bugs consistently
> + t5520: verify that `pull --rebase` shows the helpful advice when failing
>
> "git am -3" calls "git merge-recursive" when it needs to fall back
> to a three-way merge; this call has been turned into an internal
> subroutine call instead of spawning a separate subprocess.
>
> Will merge to 'master'.
>
> Eyes from other people are highly appreciated, as my eyes (and the
> original author's, too) have rotten by staring many rerolls of the
> same topic and are not effective in spotting errors.
I gave it a whirl and read over all these patches. I read them first
trying to see if there was any difference in behavior for cases where
the old code wouldn't hit a die(). As far as I can tell, the only
place that could change the non-die() paths was the early return added
when a filed couldn't be removed in remove_file_from_cache (inside the
handle_change_delete() function):
- remove_file_from_cache(path);
- update_file(o, 0, o_oid, o_mode, renamed ? renamed : path);
+ ret = remove_file_from_cache(path);
+ if (!ret)
+ ret = update_file(o, 0, o_oid, o_mode,
+ renamed ? renamed : path);
However, remove_file_from_cache() unconditionally returns 0 and
appears to have done so since it was introduced in 2005. That seems a
bit odd (Is the function just buggy? Should it be transformed into a
void function to make it clear that there's no point checking return
values?) Anyway, even if remove_file_from_cache() was modified to
return a nonzero result when its argument wasn't already in the index,
I believe that wouldn't matter here because path must be in the index
in order to reach this point of the code (if it somehow wasn't in the
index, that probably would have been a die()-worthy condition in the
old code that we just didn't bother checking for).
After my reading, I'm fairly confident that things that worked before
without hitting a die() should have the same behavior after this set
of patches (modulo the intentional changes like output buffering).
I then re-read in another pass for when the code would have hit a
die() previously, and in all those cases Johannes is returning as
early as possible and avoiding falling through to subsequent code,
which is probably the most reasonable thing to do. There might be a
few cases where a few extra steps could be taken, but it's safer to
just return early.
So, I think the series looks good. Sorry that I didn't spot any errors.
Elijah
--
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