Re: [PATCH v2 0/4] Let the builtin rebase call the git am command directly

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Fri, 18 Jan 2019, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
> 
> > Especially on Windows, where Unix shell scripting is a foreign endeavor, and
> > an expensive one at that, we really want to avoid running through the Bash.
> >
> > This not only makes everything faster, but also more robust, as the Bash we
> > use on Windows relies on a derivative of the Cygwin runtime, which in turn
> > has to jump through a couple of hoops that are sometimes a little too tricky
> > to make things work. Read: the less we rely on Unix shell scripting, the
> > more likely Windows users will be able to enjoy our software.
> >
> > Changes since v1:
> >
> >  * Rebased on top of master to avoid merge conflicts.
> 
> I do not appreciate this very much.  
> 
> We already have a good resolution prepared when merging this to
> either 'next' or 'master', which also resolves conflict with the
> other topic that requires this topic to add "--topo-order" in its
> call.  Rebasing series means invalidating the previous work recorded
> in rerere.

You misunderstand. The PR had merge conflicts with `master`. As such, the
Azure Pipeline that tests it on Windows, macOS and Linux could not give me
enough confidence. So I *had* to rebase.

Besides, I think it is a terrible idea to leave the resolution of merge
conflicts to you. You are not in the best position to judge how to resolve
them, and as a consequence you spend more time and effort on this than
necessary. Of course, it's your call if you want to do it, as a
contributor I'd rather be certain that *I* resolved the conflicts.

Ciao,
Dscho

> 	side note. The rerere database entry can be recovered from
> 	master..pu with contrib/rerere-train.sh).
> 
> >  * Adjusted the commit message talking about double entries, to clarify that
> >    it talks about HEAD's reflog.
> 
> Good.
> 
> >  * Replaced a misleading action parameter "checkout" for the reset_head() 
> >    function by the empty string: we do not check out here, we just update
> >    the refs, and certainly do not want any checkout functionality (such as
> >    hooks) to be involved.
> 
> OK.
> 
> >  * Reused a just-prepared refs_only variable instead of repeating the value
> >    assigned to it.
> 
> OK.
> 
> >  * Fixed a stale comment about the shell variable "$upstream" (which should
> >    have been negated to begin with).
> >  * Fixed error messages when files could not be opened.
> 
> Good.
> 
> Will take a look.  Thanks.
> 
>