Re: [PATCH v13 0/2] checkout: --track=fetch

[Junio C Hamano <gitster@pobox.com>]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  * Create a preparatory commit that exposes find_tracking_remote_for_ref()
>    and advise_ambiguous_fetch_refspec() from branch.c, so checkout can reuse
>    the same lookup git branch --track uses.
>  * Use advise_ambiguous_fetch_refspec() for the "multiple remotes match"
>    case, so the wording matches git branch --track.
>
> Harald Nordgren (2):
>   branch: expose helpers for finding the remote owning a tracking ref
>   checkout: extend --track with a "fetch" mode to refresh start-point
>
>  Documentation/git-checkout.adoc |  17 +-
>  Documentation/git-switch.adoc   |   5 +-
>  branch.c                        |  96 ++++++-----
>  branch.h                        |  16 ++
>  builtin/checkout.c              | 139 +++++++++++++++-
>  t/t7201-co.sh                   | 276 ++++++++++++++++++++++++++++++++
>  6 files changed, 498 insertions(+), 51 deletions(-)

I was scanning "What's cooking" and this topic was the oldest one
among the ones marked as "Needs review".  Nobody seems to have
commented on this iteration.

I am still not convinced that it is a good idea to allow "checkout"
to go to the network and muck with remote-tracking branches.  The
remote-tracking branches are meant to give us solid reference
points, and such an on-demand update to move them (which by itself
is not bad) and then use the updated result without first seeing
what it contains (which is the part I disagree with) cannot lead us
to a good place. I suspect that the feature encourages a bad
workflow to our end-users.

Having said all that, the changes since v12, in response to earlier
review comments to avoid duplicating the remote lookup and ambiguity
advice logic, look well executed in this round.  This also ensures
consistent error messages and behavior between 'git branch --track'
and 'git checkout --track=fetch'.

IOW, I find the mechanical implementation fairly solid.  I am not
sure if we are implementing a good thing, though.

One small thing about [1/2];

diff --git a/branch.h b/branch.h
index 3dc6e2a0ff..0aafa1673f 100644
--- a/branch.h
+++ b/branch.h
@@ -1,9 +1,25 @@
 #ifndef BRANCH_H
 #define BRANCH_H
 
+#include "refspec.h"
+#include "string-list.h"
+
 struct repository;
 struct strbuf;
 
+struct tracking {
+	struct refspec_item spec;
+	struct string_list *srcs;
+	const char *remote;
+	int matches;
+};
+
+void find_tracking_remote_for_ref(struct tracking *tracking,
+				  struct string_list *ambiguous_remotes);
+
+void advise_ambiguous_fetch_refspec(const char *dst,
+				    const struct string_list *ambiguous_remotes);
+

As we are not embedding any "string_list" instance into any of our
struct (we only have a pointer to a struct), unlike the way we embed
"struct refspec_item" that requires us to include "refspec.h", we do
not need to include "string-list.h".  Instead, we only need to
declare "struct string_list", just like we declare repository and
strbuf.

diff --git i/branch.h w/branch.h
index 0aafa1673f..c2e6725491 100644
--- i/branch.h
+++ w/branch.h
@@ -2,8 +2,8 @@
 #define BRANCH_H
 
 #include "refspec.h"
-#include "string-list.h"
 
+struct string_list;
 struct repository;
 struct strbuf;