Re: [PATCH 01/14] pull: implement fetch + merge

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: May 18, 2015, 15:55 UTC
Message-ID: <8fdf250d93dc34505fc77f01b415a43b@www.dscho.org>
In-Reply-To: <1431961571-20370-2-git-send-email-pyokagan@gmail.com>

Hi Paul,

On 2015-05-18 17:05, Paul Tan wrote:

Show 79 quoted lines

> Start off the rewrite of git-pull.sh to C by implementing a simple
> git-pull builtin that:
> 
> 1. Parses the command line arguments into [<repo> [<refspecs>...]]
> 
> 2. Runs git-fetch, passing the repo and refspecs to it.
> 
> 3. Runs git-merge FETCH_HEAD.
> 
> Due to missing features, several tests are disabled:
> 
> * Passing flags to git fetch, rebase, merge:
> 
>   * t5521: verbosity flags -v, -s
> 
> * Passing flags to git-merge, git-rebase:
> 
>   * t5150, t5572: Pass --ff, --no-ff, --ff-only to git-merge
> 
>   * t5520, t5521, t7406: Support --rebase, branch.*.rebase and
>     pull.rebase config
> 
>   * t6029, t4013: -s/--strategy
> 
>   * t6037: -X/--strategy-option
> 
>   * t5524: --log
> 
> * Passing flags to git-fetch:
> 
>   * t5521: --dry-run
> 
>   * t5500: --depth
> 
>   * t5521: --force
> 
>   * t5521: --all
> 
>   * t7403: --no-recurse-submodules
> 
> * t7601: pull.ff config sets --ff, --no-ff, --ff-only
> 
> * t5520: branch.*.rebase or pull.rebase sets --rebase, --rebase=preserve
> 
> * t5520: Pulling into void
> 
> * t5520: appropriate user-friendly error messages on no merge candidates
>   case
> 
> * t5520: appropriate user-friendly error messages if index has
>   unresolved entries.
> 
> * t5520: fast-forward working tree if branch head is updated by
>   git-fetch
> 
> * t5520: set reflog action to "pull args..."
> 
> These features will be re-implemented in succeeding patches.
> 
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  Makefile                     |   2 +-
>  builtin.h                    |   1 +
>  builtin/pull.c               |  83 +++++++++++
>  contrib/examples/git-pull.sh | 339 +++++++++++++++++++++++++++++++++++++++++++
>  git-pull.sh                  | 339 -------------------------------------------
>  git.c                        |   1 +
>  t/t4013-diff-various.sh      |   3 +
>  t/t5150-request-pull.sh      |   2 +-
>  t/t5500-fetch-pack.sh        |  10 +-
>  t/t5520-pull.sh              |  68 ++++-----
>  t/t5521-pull-options.sh      |  20 +--
>  t/t5524-pull-msg.sh          |   2 +-
>  t/t5572-pull-submodule.sh    |   4 +
>  t/t6029-merge-subtree.sh     |   6 +-
>  t/t6037-merge-ours-theirs.sh |   2 +-
>  t/t7403-submodule-sync.sh    |   8 +-
>  t/t7406-submodule-update.sh  |   2 +
>  t/t7601-merge-pull-config.sh |   4 +-

This is an excellent way to develop the patch series. For the final patch series, I would like to propose that `git-pull.sh` is replaced by the builtin with the very last patch, though. That way, the builtin can be implemented in the way you did, but without touching the tests. In the final form, the builtin git-pull will be a drop-in replacement for the shell script, and the final patch will just make that switch.

In other words, patch 1/N would leave SCRIPT_SH in the Makefile untouched, and leave out the "pull" line from `git.c`.

Oh, by the way, if you pass the `-M` option to `git format-patch`, the diffstat will report the move rather than an added and a deleted git-pull.sh.

Ciao, Dscho