Re: [PATCH v3 4/4] transport.c: introduce core.alternateRefsPrefixes

[Jeff King]

On Thu, Sep 27, 2018 at 09:25:45PM -0700, Taylor Blau wrote:

> The recently-introduced "core.alternateRefsCommand" allows callers to
> specify with high flexibility the tips that they wish to advertise from
> alternates. This flexibility comes at the cost of some inconvenience
> when the caller only wishes to limit the advertisement to one or more
> prefixes.
> 
> For example, to advertise only tags, a caller using
> 'core.alternateRefsCommand' would have to do:
> 
>   $ git config core.alternateRefsCommand ' \
>   git -C "$1" for-each-ref refs/tags --format="%(objectname)"'

This has the same "$@" issue as the previous one, I think (which only
makes your point about it being cumbersome more true!).

> In the case that the caller wishes to specify multiple prefixes, they
> may separate them by whitespace. If "core.alternateRefsCommand" is set,
> it will take precedence over "core.alternateRefsPrefixes".

Just a meta-comment: I don't particularly mind this discussion in the
commit message, but since these points ought to be in the documentation
anyway, it may make sense to omit them here in the name of brevity.

> +core.alternateRefsPrefixes::
> + When listing references from an alternate, list only references that 
> begin
> + with the given prefix. Prefixes match as if they were given as 
> arguments to
> + linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them 
> with
> + whitespace. If `core.alternateRefsCommand` is set, setting
> + `core.alternateRefsPrefixes` has no effect.

Looks good.

> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> index 503dde35a4..3449967cc7 100755
> --- a/t/t5410-receive-pack.sh
> +++ b/t/t5410-receive-pack.sh
> @@ -46,4 +46,12 @@ test_expect_success 'with core.alternateRefsCommand' '
>   test_cmp expect actual.haves
>  '
>  
> +test_expect_success 'with core.alternateRefsPrefixes' '
> + test_config -C fork core.alternateRefsPrefixes "refs/tags" &&
> + git rev-parse one three two >expect &&
> + printf "" | git receive-pack fork >actual &&
> + extract_haves actual.haves &&
> + test_cmp expect actual.haves
> +'

If you follow my suggestion on the test setup from the last patch, it
would make sense to just put "refs/heads/public/" here. Although neither
that nor what you have here tests the whitespace separation. Possibly
there should be a third hierarchy.

> diff --git a/transport.c b/transport.c
> index e271b66603..83474add28 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct 
> child_process *cmd,
>   argv_array_pushf(>args, "--git-dir=%s", repo_path);
>   argv_array_push(>args, "for-each-ref");
>   argv_array_push(>args, "--format=%(objectname)");
> +
> + if (!git_config_get_value("core.alternateRefsPrefixes", 
> )) {
> + argv_array_push(>args, "--");
> + argv_array_split(>args, value);
> + }

And this part looks good.

-Peff

Re: [PATCH v3 3/4] transport.c: introduce core.alternateRefsCommand

[Jeff King]

On Thu, Sep 27, 2018 at 09:25:42PM -0700, Taylor Blau wrote:

> Let the repository that has alternates configure this command to avoid
> trusting the alternate to provide us a safe command to run in the shell.
> To behave differently on each alternate (e.g., only list tags from
> alternate A, only heads from B) provide the path of the alternate as the
> first argument.

Well, you also need to pass the path so it knows which repo to look at.
Which I think is the primary reason we do it, but behaving differently
for each alternate is another option.

> +core.alternateRefsCommand::
> + When advertising tips of available history from an alternate, use the 
> shell to
> + execute the specified command instead of linkgit:git-for-each-ref[1]. 
> The
> + first argument is the absolute path of the alternate. Output must be of 
> the
> + form: `%(objectname)`, where multiple tips are separated by newlines.

I wonder if people may be confused about the %(objectname) syntax, since
it's specific to for-each-ref.  Now that we've simplified the output
format to a single value, perhaps we should define it more directly.
E.g., like:

  The output should contain one hex object id per line (i.e., the same
  as produced by `git for-each-ref --format='%(objectname)'`).

Now that we've dropped the refname requirement from the output, it is
more clear that this really does not have to be about refs at all.  In
the most technical sense, what we really allow in the output is any
object id X for which the alternate promises it has all objects
reachable from X. Ref tips are a convenient and efficient way of
providing that, but they are not the only possibility (and likewise, it
is fine to omit duplicates or even tips that are ancestors of other
tips).

I think that's probably getting _too_ technical, though. It probably
makes sense to just keep thinking of these as "what are the ref tips".

> +This is useful when a repository only wishes to advertise some of its
> +alternate's references as ".have"'s. For example, to only advertise branch

Maybe put ".have" into backticks for formatting?

> +heads, configure `core.alternateRefsCommand` to the path of a script which 
> runs
> +`git --git-dir="$1" for-each-ref --format='%(objectname)' refs/heads`.

Does that script actually work? Because of the way we invoke shell
commands with arguments, I think we'd end up with:

  git --git-dir="$1" for-each-ref --format='%(objectname)' refs/heads "$@"

Possibly for-each-ref would ignore the extra path argument (thinking
it's a ref pattern that just doesn't match), but it's definitely not
what you intended. You'd have to write:

  f() { git --git-dir=$1 ...etc; } f

in the usual way. That's a minor pain, but it's what makes the more
direct:

  /my/script

work.

The other alternative is to pass $GIT_DIR in the environment on behalf
of the program. Then writing:

  git for-each-ref --format='%(objectname)' refs/heads

would Just Work. But it's a bit subtle, since it is not immediately
obvious that the command is meant to run in a different repository.

> diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
> new file mode 100755
> index 00..503dde35a4
> --- /dev/null
> +++ b/t/t5410-receive-pack.sh
> @@ -0,0 +1,49 @@
> +#!/bin/sh
> +
> +test_description='git receive-pack test'

The name of this test file and the description are pretty vague. Can we
say something like "test handling of receive-pack with alternate-refs
config"?

> +test_expect_success 'setup' '
> + test_commit one &&
> + git update-ref refs/heads/a HEAD &&
> + test_commit two &&
> + git update-ref refs/heads/b HEAD &&
> + test_commit three &&
> + git update-ref refs/heads/c HEAD &&
> + git clone --bare . fork &&
> + git clone fork pusher &&
> + (
> + cd fork &&
> + git update-ref --stdin <<-\EOF &&
> + delete refs/heads/a
> + delete refs/heads/b
> + delete refs/heads/c
> + delete refs/heads/master
> + delete refs/tags/one
> + delete refs/tags/two
> + delete refs/tags/three
> + EOF
> + echo "../../.git/objects" >objects/info/alternates
> + )
> +'

This setup is kind of convoluted. You're deleting those refs in the
fork, I think, because we don't want them to suppress the duplicate
.have lines from the alternate. Might it be easier to just create the
.have lines we're interested in after the fact?

I think we can also use "clone -s" to make the setup of the alternate a
little simpler.

I don't see the "pusher" repo being used for anything here. Leftover
cruft from when you were using "git push" to test?

So all together, perhaps something like:

  # we have a fork which points back to us as an alternate
  test_commit base &&
  git clone -s . fork &&

  # the alternate has two refs with new tips, in two separate hierarchies
  git checkout -b public/branch master &&
  test_commit 

Re: [PATCH v3 2/4] transport.c: extract 'fill_alternate_refs_command'

[Jeff King]

On Thu, Sep 27, 2018 at 09:25:39PM -0700, Taylor Blau wrote:

> To list alternate references, 'read_alternate_refs' creates a child
> process running 'git for-each-ref' in the alternate's Git directory.
> 
> Prepare to run other commands besides 'git for-each-ref' by introducing
> and moving the relevant code from 'read_alternate_refs' to
> 'fill_alternate_refs_command'.
> 
> Signed-off-by: Taylor Blau 
> ---
>  transport.c | 18 --
>  1 file changed, 12 insertions(+), 6 deletions(-)

Same as before, but moving the slightly modified code. Makes sense.

-Peff

Re: [PATCH v3 1/4] transport: drop refnames from for_each_alternate_ref

[Jeff King]

> From: Jeff King 

Pretty sure that isn't right. :)

The preferred way to send a patch with a different author is to have
actual email be "From:" you, but then include a:

  From: Jeff King 

as the first line of the body (which git-am will then pick up).
git-send-email will do this for you automatically. Other scripts (like
say, if you're sending the output of format-patch into mutt) used to
have to implement it themselves, but these days we have "format-patch
--from", which should directly output what you want.

> ---
>  builtin/receive-pack.c | 3 +--
>  fetch-pack.c   | 3 +--
>  transport.c| 6 +++---
>  transport.h| 2 +-
>  4 files changed, 6 insertions(+), 8 deletions(-)

The patch itself is flawless, of course. ;)

-Peff

[PATCH 1/1] roll wt_status_state into wt_status and populate in the collect phase

[Stephen P. Smith]

When updating the collect and print functions, it was found that
status variables were initialized in the collect phase and some
variables were later freed in the print functions.

Move the status state structure variables into the status state
structure and populate them in the collect functions.

Create a new funciton to free the buffers that were being freed in the
print function.  Call this new function in commit.c where both the
collect and print functions were being called.

Based on a patch suggestion by Junio C Hamano. [1]

[1] https://public-inbox.org/git/xmqqr2i5ueg4@gitster-ct.c.googlers.com/

Signed-off-by: Stephen P. Smith 
---
 builtin/commit.c |   3 ++
 wt-status.c  | 135 +--
 wt-status.h  |  38 ++---
 3 files changed, 83 insertions(+), 93 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 51ecebbec1..e168321e49 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -506,6 +506,7 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
 
wt_status_collect(s);
wt_status_print(s);
+   wt_status_collect_free_buffers(s);
 
return s->committable;
 }
@@ -1388,6 +1389,8 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
s.prefix = prefix;
 
wt_status_print();
+   wt_status_collect_free_buffers();
+
return 0;
 }
 
diff --git a/wt-status.c b/wt-status.c
index c7f76d4758..9977f0cdf2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -744,21 +744,26 @@ static int has_unmerged(struct wt_status *s)
 
 void wt_status_collect(struct wt_status *s)
 {
-   struct wt_status_state state;
wt_status_collect_changes_worktree(s);
-
if (s->is_initial)
wt_status_collect_changes_initial(s);
else
wt_status_collect_changes_index(s);
wt_status_collect_untracked(s);
 
-   memset(, 0, sizeof(state));
-   wt_status_get_state(, s->branch && !strcmp(s->branch, "HEAD"));
-   if (state.merge_in_progress && !has_unmerged(s))
+   wt_status_get_state(>state, s->branch && !strcmp(s->branch, "HEAD"));
+   if (s->state.merge_in_progress && !has_unmerged(s))
s->committable = 1;
 }
 
+void wt_status_collect_free_buffers(struct wt_status *s)
+{
+   free(s->state.branch);
+   free(s->state.onto);
+   free(s->state.detached_from);
+}
+
+
 static void wt_longstatus_print_unmerged(struct wt_status *s)
 {
int shown_header = 0;
@@ -1087,8 +1092,7 @@ static void wt_longstatus_print_tracking(struct wt_status 
*s)
 }
 
 static void show_merge_in_progress(struct wt_status *s,
-   struct wt_status_state *state,
-   const char *color)
+  const char *color)
 {
if (has_unmerged(s)) {
status_printf_ln(s, color, _("You have unmerged paths."));
@@ -1109,16 +1113,15 @@ static void show_merge_in_progress(struct wt_status *s,
 }
 
 static void show_am_in_progress(struct wt_status *s,
-   struct wt_status_state *state,
const char *color)
 {
status_printf_ln(s, color,
_("You are in the middle of an am session."));
-   if (state->am_empty_patch)
+   if (s->state.am_empty_patch)
status_printf_ln(s, color,
_("The current patch is empty."));
if (s->hints) {
-   if (!state->am_empty_patch)
+   if (!s->state.am_empty_patch)
status_printf_ln(s, color,
_("  (fix conflicts and then run \"git am 
--continue\")"));
status_printf_ln(s, color,
@@ -1242,10 +1245,9 @@ static int read_rebase_todolist(const char *fname, 
struct string_list *lines)
 }
 
 static void show_rebase_information(struct wt_status *s,
-   struct wt_status_state *state,
-   const char *color)
+   const char *color)
 {
-   if (state->rebase_interactive_in_progress) {
+   if (s->state.rebase_interactive_in_progress) {
int i;
int nr_lines_to_show = 2;
 
@@ -1296,28 +1298,26 @@ static void show_rebase_information(struct wt_status *s,
 }
 
 static void print_rebase_state(struct wt_status *s,
-   struct wt_status_state *state,
-   const char *color)
+  const char *color)
 {
-   if (state->branch)
+   if (s->state.branch)
status_printf_ln(s, color,
 _("You are currently rebasing branch '%s' on 
'%s'."),
-state->branch,
-state->onto);
+s->state.branch,
+

[PATCH 0/1] wt-status-state-cleanup

[Stephen P. Smith]

Junio suggested a cleanup patch, jc/wt-status-state-cleanup, which is
the basis for this patch.

This patch uses ss/wt-status-committable.

The main update from the patch suggestion was cleanup of the free
calls for three strings in the status structure.

Stephen P. Smith (1):
  roll wt_status_state into wt_status and populate in the collect phase

 builtin/commit.c |   3 ++
 wt-status.c  | 135 +--
 wt-status.h  |  38 ++---
 3 files changed, 83 insertions(+), 93 deletions(-)

-- 
2.19.0

[PATCH v3 4/4] transport.c: introduce core.alternateRefsPrefixes

[Taylor Blau]

The recently-introduced "core.alternateRefsCommand" allows callers to
specify with high flexibility the tips that they wish to advertise from
alternates. This flexibility comes at the cost of some inconvenience
when the caller only wishes to limit the advertisement to one or more
prefixes.

For example, to advertise only tags, a caller using
'core.alternateRefsCommand' would have to do:

  $ git config core.alternateRefsCommand ' \
  git -C "$1" for-each-ref refs/tags --format="%(objectname)"'

The above is cumbersome to write, so let's introduce a
"core.alternateRefsPrefixes" to address this common case. Instead, the
caller can run:

  $ git config core.alternateRefsPrefixes 'refs/tags'

Which will behave identically to the longer example using
"core.alternateRefsCommand".

Since the value of "core.alternateRefsPrefixes" is appended to 'git
for-each-ref' and then executed, include a "--" before taking the
configured value to avoid misinterpreting arguments as flags to 'git
for-each-ref'.

In the case that the caller wishes to specify multiple prefixes, they
may separate them by whitespace. If "core.alternateRefsCommand" is set,
it will take precedence over "core.alternateRefsPrefixes".

Signed-off-by: Taylor Blau 
---
 Documentation/config.txt | 7 +++
 t/t5410-receive-pack.sh  | 8 
 transport.c  | 5 +
 3 files changed, 20 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index afcb18331a..9ef792ef0d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -627,6 +627,13 @@ alternate's references as ".have"'s. For example, to only 
advertise branch
 heads, configure `core.alternateRefsCommand` to the path of a script which runs
 `git --git-dir="$1" for-each-ref --format='%(objectname)' refs/heads`.
 
+core.alternateRefsPrefixes::
+   When listing references from an alternate, list only references that 
begin
+   with the given prefix. Prefixes match as if they were given as 
arguments to
+   linkgit:git-for-each-ref[1]. To list multiple prefixes, separate them 
with
+   whitespace. If `core.alternateRefsCommand` is set, setting
+   `core.alternateRefsPrefixes` has no effect.
+
 core.bare::
If true this repository is assumed to be 'bare' and has no
working directory associated with it.  If this is the case a
diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
index 503dde35a4..3449967cc7 100755
--- a/t/t5410-receive-pack.sh
+++ b/t/t5410-receive-pack.sh
@@ -46,4 +46,12 @@ test_expect_success 'with core.alternateRefsCommand' '
test_cmp expect actual.haves
 '
 
+test_expect_success 'with core.alternateRefsPrefixes' '
+   test_config -C fork core.alternateRefsPrefixes "refs/tags" &&
+   git rev-parse one three two >expect &&
+   printf "" | git receive-pack fork >actual &&
+   extract_haves actual.haves &&
+   test_cmp expect actual.haves
+'
+
 test_done
diff --git a/transport.c b/transport.c
index e271b66603..83474add28 100644
--- a/transport.c
+++ b/transport.c
@@ -1341,6 +1341,11 @@ static void fill_alternate_refs_command(struct 
child_process *cmd,
argv_array_pushf(>args, "--git-dir=%s", repo_path);
argv_array_push(>args, "for-each-ref");
argv_array_push(>args, "--format=%(objectname)");
+
+   if (!git_config_get_value("core.alternateRefsPrefixes", 
)) {
+   argv_array_push(>args, "--");
+   argv_array_split(>args, value);
+   }
}
 
cmd->env = local_repo_env;
-- 
2.19.0.221.g150f307af

[PATCH v3 1/4] transport: drop refnames from for_each_alternate_ref

[Jeff King]

None of the current callers use the refname parameter we pass to their
callbacks. In theory somebody _could_ do so, but it's actually quite
weird if you think about it: it's a ref in somebody else's repository.
So the name has no meaning locally, and in fact there may be duplicates
if there are multiple alternates.

The users of this interface really only care about seeing some ref tips,
since that promises that the alternate has the full commit graph
reachable from there. So let's keep the information we pass back to the
bare minimum.

Signed-off-by: Jeff King 
---
 builtin/receive-pack.c | 3 +--
 fetch-pack.c   | 3 +--
 transport.c| 6 +++---
 transport.h| 2 +-
 4 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 4d30001950..6792291f5e 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -281,8 +281,7 @@ static int show_ref_cb(const char *path_full, const struct 
object_id *oid,
return 0;
 }
 
-static void show_one_alternate_ref(const char *refname,
-  const struct object_id *oid,
+static void show_one_alternate_ref(const struct object_id *oid,
   void *data)
 {
struct oidset *seen = data;
diff --git a/fetch-pack.c b/fetch-pack.c
index 75047a4b2a..b643de143b 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -76,8 +76,7 @@ struct alternate_object_cache {
size_t nr, alloc;
 };
 
-static void cache_one_alternate(const char *refname,
-   const struct object_id *oid,
+static void cache_one_alternate(const struct object_id *oid,
void *vcache)
 {
struct alternate_object_cache *cache = vcache;
diff --git a/transport.c b/transport.c
index 1c76d64aba..2e0bc414d0 100644
--- a/transport.c
+++ b/transport.c
@@ -1336,7 +1336,7 @@ static void read_alternate_refs(const char *path,
cmd.git_cmd = 1;
argv_array_pushf(, "--git-dir=%s", path);
argv_array_push(, "for-each-ref");
-   argv_array_push(, "--format=%(objectname) %(refname)");
+   argv_array_push(, "--format=%(objectname)");
cmd.env = local_repo_env;
cmd.out = -1;
 
@@ -1348,13 +1348,13 @@ static void read_alternate_refs(const char *path,
struct object_id oid;
 
if (get_oid_hex(line.buf, ) ||
-   line.buf[GIT_SHA1_HEXSZ] != ' ') {
+   line.buf[GIT_SHA1_HEXSZ]) {
warning(_("invalid line while parsing alternate refs: 
%s"),
line.buf);
break;
}
 
-   cb(line.buf + GIT_SHA1_HEXSZ + 1, , data);
+   cb(, data);
}
 
fclose(fh);
diff --git a/transport.h b/transport.h
index 01e717c29e..9baeca2d7a 100644
--- a/transport.h
+++ b/transport.h
@@ -261,6 +261,6 @@ int transport_refs_pushed(struct ref *ref);
 void transport_print_push_status(const char *dest, struct ref *refs,
  int verbose, int porcelain, unsigned int *reject_reasons);
 
-typedef void alternate_ref_fn(const char *refname, const struct object_id 
*oid, void *);
+typedef void alternate_ref_fn(const struct object_id *oid, void *);
 extern void for_each_alternate_ref(alternate_ref_fn, void *);
 #endif
-- 
2.19.0.221.g150f307af

[PATCH v3 0/4] Filter alternate references

[Taylor Blau]

Hi,

Attached is the third re-roll of mine and Peff's series to introduce
'core.alternateRefsCommand', and 'core.alternateRefsPrefixes' to filter
the initial ".have" advertisement when an alternate has a pathologically
large number of references.

A range-diff against v2 is included below, but the major changes between
the two revisions are as follows:

  1. Documentation and testing clean-up, per helpful input from Junio,
 Peff, and brian carlson.

  2. Included also is a preparatory patch from Peff, to change the
 requirement that we provide refnames for alternate references. We
 no longer allow this, and the first commit sent makes that such
 change.

I imagine that we may hit one more re-roll, depending on the outcome of
this review. The series has not fundamentally changed since v2, so I
think that we are at a point of stasis there. Anything that is left
outstanding from v3 should hopefully be similarly-not-earth-shattering
;-).

Thanks in advance for your review.

Thanks,
Taylor

Jeff King (1):
  transport: drop refnames from for_each_alternate_ref

Taylor Blau (3):
  transport.c: extract 'fill_alternate_refs_command'
  transport.c: introduce core.alternateRefsCommand
  transport.c: introduce core.alternateRefsPrefixes

 Documentation/config.txt | 18 +
 builtin/receive-pack.c   |  3 +--
 fetch-pack.c |  3 +--
 t/t5410-receive-pack.sh  | 57 
 transport.c  | 38 +--
 transport.h  |  2 +-
 6 files changed, 108 insertions(+), 13 deletions(-)
 create mode 100755 t/t5410-receive-pack.sh

Range-diff against v2:
-:  -- > 1:  037273dab0 transport: drop refnames from 
for_each_alternate_ref
1:  6e3a58afe7 ! 2:  9479470cb1 transport.c: extract 
'fill_alternate_refs_command'
@@ -24,7 +24,7 @@
 +  cmd->git_cmd = 1;
 +  argv_array_pushf(>args, "--git-dir=%s", repo_path);
 +  argv_array_push(>args, "for-each-ref");
-+  argv_array_push(>args, "--format=%(objectname) %(refname)");
++  argv_array_push(>args, "--format=%(objectname)");
 +  cmd->env = local_repo_env;
 +  cmd->out = -1;
 +}
@@ -39,7 +39,7 @@
 -  cmd.git_cmd = 1;
 -  argv_array_pushf(, "--git-dir=%s", path);
 -  argv_array_push(, "for-each-ref");
--  argv_array_push(, "--format=%(objectname) %(refname)");
+-  argv_array_push(, "--format=%(objectname)");
 -  cmd.env = local_repo_env;
 -  cmd.out = -1;
 +  fill_alternate_refs_command(, path);
2:  9797f52551 ! 3:  2dbcd54190 transport.c: introduce core.alternateRefsCommand
@@ -3,24 +3,24 @@
 transport.c: introduce core.alternateRefsCommand

 When in a repository containing one or more alternates, Git would
-sometimes like to list references from its alternates. For example, 
'git
-receive-pack' list the objects pointed to by alternate references as
-special ".have" references.
+sometimes like to list references from those alternates. For example,
+'git receive-pack' lists the "tips" pointed to by references in those
+alternates as special ".have" references.

 Listing ".have" references is designed to make pushing changes from
 upstream to a fork a lightweight operation, by advertising to the 
pusher
 that the fork already has the objects (via its alternate). Thus, the
 client can avoid sending them.

-However, when the alternate has a pathologically large number of
-references, the initial advertisement is too expensive. In fact, it can
-dominate any such optimization where the pusher avoids sending certain
-objects.
+However, when the alternate (upstream, in the previous example) has a
+pathologically large number of references, the initial advertisement is
+too expensive. In fact, it can dominate any such optimization where the
+pusher avoids sending certain objects.

 Introduce "core.alternateRefsCommand" in order to provide a facility to
 limit or filter alternate references. This can be used, for example, to
-filter out "uninteresting" references from the initial advertisement in
-the above scenario.
+filter out references the alternate does not wish to send (for space
+concerns, or otherwise) during the initial advertisement.

 Let the repository that has alternates configure this command to avoid
 trusting the alternate to provide us a safe command to run in the 
shell.
@@ -38,15 +38,15 @@
expect HEAD to be a symbolic link.

 +core.alternateRefsCommand::
-+  When listing references from an alternate (e.g., in the case of 
".have"), use
-+  the shell to execute the specified command instead of
-+  linkgit:git-for-each-ref[1]. The first argument is the path of the 
alternate.
-+  Output must be of the form: `%(objectname) SPC %(refname)`.

[PATCH v3 3/4] transport.c: introduce core.alternateRefsCommand

[Taylor Blau]

When in a repository containing one or more alternates, Git would
sometimes like to list references from those alternates. For example,
'git receive-pack' lists the "tips" pointed to by references in those
alternates as special ".have" references.

Listing ".have" references is designed to make pushing changes from
upstream to a fork a lightweight operation, by advertising to the pusher
that the fork already has the objects (via its alternate). Thus, the
client can avoid sending them.

However, when the alternate (upstream, in the previous example) has a
pathologically large number of references, the initial advertisement is
too expensive. In fact, it can dominate any such optimization where the
pusher avoids sending certain objects.

Introduce "core.alternateRefsCommand" in order to provide a facility to
limit or filter alternate references. This can be used, for example, to
filter out references the alternate does not wish to send (for space
concerns, or otherwise) during the initial advertisement.

Let the repository that has alternates configure this command to avoid
trusting the alternate to provide us a safe command to run in the shell.
To behave differently on each alternate (e.g., only list tags from
alternate A, only heads from B) provide the path of the alternate as the
first argument.

Signed-off-by: Taylor Blau 
---
 Documentation/config.txt | 11 +
 t/t5410-receive-pack.sh  | 49 
 transport.c  | 19 
 3 files changed, 75 insertions(+), 4 deletions(-)
 create mode 100755 t/t5410-receive-pack.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ad0f4510c3..afcb18331a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -616,6 +616,17 @@ core.preferSymlinkRefs::
This is sometimes needed to work with old scripts that
expect HEAD to be a symbolic link.
 
+core.alternateRefsCommand::
+   When advertising tips of available history from an alternate, use the 
shell to
+   execute the specified command instead of linkgit:git-for-each-ref[1]. 
The
+   first argument is the absolute path of the alternate. Output must be of 
the
+   form: `%(objectname)`, where multiple tips are separated by newlines.
++
+This is useful when a repository only wishes to advertise some of its
+alternate's references as ".have"'s. For example, to only advertise branch
+heads, configure `core.alternateRefsCommand` to the path of a script which runs
+`git --git-dir="$1" for-each-ref --format='%(objectname)' refs/heads`.
+
 core.bare::
If true this repository is assumed to be 'bare' and has no
working directory associated with it.  If this is the case a
diff --git a/t/t5410-receive-pack.sh b/t/t5410-receive-pack.sh
new file mode 100755
index 00..503dde35a4
--- /dev/null
+++ b/t/t5410-receive-pack.sh
@@ -0,0 +1,49 @@
+#!/bin/sh
+
+test_description='git receive-pack test'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+   test_commit one &&
+   git update-ref refs/heads/a HEAD &&
+   test_commit two &&
+   git update-ref refs/heads/b HEAD &&
+   test_commit three &&
+   git update-ref refs/heads/c HEAD &&
+   git clone --bare . fork &&
+   git clone fork pusher &&
+   (
+   cd fork &&
+   git update-ref --stdin <<-\EOF &&
+   delete refs/heads/a
+   delete refs/heads/b
+   delete refs/heads/c
+   delete refs/heads/master
+   delete refs/tags/one
+   delete refs/tags/two
+   delete refs/tags/three
+   EOF
+   echo "../../.git/objects" >objects/info/alternates
+   )
+'
+
+extract_haves () {
+   depacketize | perl -lne '/^(\S+) \.have/ and print $1'
+}
+
+test_expect_success 'with core.alternateRefsCommand' '
+   write_script fork/alternate-refs <<-\EOF &&
+   git --git-dir="$1" for-each-ref \
+   --format="%(objectname)" \
+   refs/heads/a \
+   refs/heads/c
+   EOF
+   test_config -C fork core.alternateRefsCommand alternate-refs &&
+   git rev-parse a c >expect &&
+   printf "" | git receive-pack fork >actual &&
+   extract_haves actual.haves &&
+   test_cmp expect actual.haves
+'
+
+test_done
diff --git a/transport.c b/transport.c
index 2825debac5..e271b66603 100644
--- a/transport.c
+++ b/transport.c
@@ -1328,10 +1328,21 @@ char *transport_anonymize_url(const char *url)
 static void fill_alternate_refs_command(struct child_process *cmd,
const char *repo_path)
 {
-   cmd->git_cmd = 1;
-   argv_array_pushf(>args, "--git-dir=%s", repo_path);
-   argv_array_push(>args, "for-each-ref");
-   argv_array_push(>args, "--format=%(objectname)");
+   const char *value;
+
+   if (!git_config_get_value("core.alternateRefsCommand", 

[PATCH v3 2/4] transport.c: extract 'fill_alternate_refs_command'

[Taylor Blau]

To list alternate references, 'read_alternate_refs' creates a child
process running 'git for-each-ref' in the alternate's Git directory.

Prepare to run other commands besides 'git for-each-ref' by introducing
and moving the relevant code from 'read_alternate_refs' to
'fill_alternate_refs_command'.

Signed-off-by: Taylor Blau 
---
 transport.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/transport.c b/transport.c
index 2e0bc414d0..2825debac5 100644
--- a/transport.c
+++ b/transport.c
@@ -1325,6 +1325,17 @@ char *transport_anonymize_url(const char *url)
return xstrdup(url);
 }
 
+static void fill_alternate_refs_command(struct child_process *cmd,
+   const char *repo_path)
+{
+   cmd->git_cmd = 1;
+   argv_array_pushf(>args, "--git-dir=%s", repo_path);
+   argv_array_push(>args, "for-each-ref");
+   argv_array_push(>args, "--format=%(objectname)");
+   cmd->env = local_repo_env;
+   cmd->out = -1;
+}
+
 static void read_alternate_refs(const char *path,
alternate_ref_fn *cb,
void *data)
@@ -1333,12 +1344,7 @@ static void read_alternate_refs(const char *path,
struct strbuf line = STRBUF_INIT;
FILE *fh;
 
-   cmd.git_cmd = 1;
-   argv_array_pushf(, "--git-dir=%s", path);
-   argv_array_push(, "for-each-ref");
-   argv_array_push(, "--format=%(objectname)");
-   cmd.env = local_repo_env;
-   cmd.out = -1;
+   fill_alternate_refs_command(, path);
 
if (start_command())
return;
-- 
2.19.0.221.g150f307af

Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree

[Sam McKelvie]

> On Sep 27, 2018, at 6:43 PM, Junio C Hamano  wrote:
> 
> Sam McKelvie  writes:
> 
 Subject: Re: [PATCH] submodule: Alllow staged changes for 
 get_superproject_working_tree
>>> 
>>> s/Alllow/allow/;
>>> 
>> 
>> Ok, no caps on first letter of subject.
> 
> Ah, that, too.  I meant to correct triple ell, though ;-)
> 
> When one reviewer says Reviewed-by: but you later found that the
> patch was not good enough to deserve a redoing, feel free to redo
> the patch and do not add the Reviewed-by: you got for the old one
> to your second submission.  The difference between the one that was
> reviewed and the one you updated invalidates the stale Reviewed-by:,
> essentially.
> 
> Some reviewers explicitly state "With this and that nit corrected or
> left as-is, the patch is Reviewed-by: me" to tell you that as long
> as the difference between the version reviewed and the updated one
> is limited within the named issues, you can add Reviewed-by: to your
> rerolled patch.
> 
> In this case, I think the nits are pretty small and I do not mind
> tweaking the version we are discussing on my end, without having you
> to send an updated one.
> 
> Here is what I'd squash into your commit, with title corrected, if
> you are OK with that plan.  In the meantime, I'll keep the following
> as a separate patch on top when I merge your fix to 'pu'.
> 
> Thanks.

I wholeheartedly approve of that plan and your tweaking commit below. Thank 
you, Junio.

~Sam

> 
> t/t1500-rev-parse.sh | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index b774cafc5d..01abee533d 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -134,6 +134,7 @@ test_expect_success 'rev-parse --is-shallow-repository in 
> non-shallow repo' '
> test_expect_success 'showing the superproject correctly' '
>   git rev-parse --show-superproject-working-tree >out &&
>   test_must_be_empty out &&
> +
>   test_create_repo super &&
>   test_commit -C super test_commit &&
>   test_create_repo sub &&
> @@ -142,20 +143,20 @@ test_expect_success 'showing the superproject 
> correctly' '
>   echo $(pwd)/super >expect  &&
>   git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
>   test_cmp expect out &&
> +
>   test_commit -C super submodule_add &&
>   git -C super checkout -b branch1 &&
>   git -C super/dir/sub checkout -b branch1 &&
>   test_commit -C super/dir/sub branch1_commit &&
>   git -C super add dir/sub &&
>   test_commit -C super branch1_commit &&
> - git -C super checkout master &&
> - git -C super checkout -b branch2 &&
> - git -C super/dir/sub checkout master &&
> - git -C super/dir/sub checkout -b branch2 &&
> + git -C super checkout -b branch2 master &&
> + git -C super/dir/sub checkout -b branch2 master &&
>   test_commit -C super/dir/sub branch2_commit &&
>   git -C super add dir/sub &&
>   test_commit -C super branch2_commit &&
>   test_must_fail git -C super merge branch1 &&
> +
>   git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
>   test_cmp expect out
> '

Re: [PATCH v2 1/5] split-index: add tests to demonstrate the racy split index problem

[SZEDER Gábor]

On Fri, Sep 28, 2018 at 02:48:43AM +0200, SZEDER Gábor wrote:
> Junio,
> 
> On Thu, Sep 27, 2018 at 02:44:30PM +0200, SZEDER Gábor wrote:
> > diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh
> > new file mode 100755
> > index 00..ebde418d7e
> > --- /dev/null
> > +++ b/t/t1701-racy-split-index.sh
> > @@ -0,0 +1,218 @@
> > +#!/bin/sh
> > +
> > +# This test can give false success if your machine is sufficiently
> > +# slow or all trials happened to happen on second boundaries.
> > +
> > +test_description='racy split index'
> > +
> > +. ./test-lib.sh
> > +
> > +test_expect_success 'setup' '
> > +   # Only split the index when the test explicitly says so.
> > +   sane_unset GIT_TEST_SPLIT_INDEX GIT_FSMONITOR_TEST &&
> 
> Please note that this patch adds another use of the environment
> variable GIT_FSMONITOR_TEST, while the topic 'bp/rename-test-env-var'
> is about to rename that variable to GIT_TEST_FSMONITOR.

Hang on for a sec.  I unset GIT_FSMONITOR_TEST in this test, because I
saw that 't1700-split-index.sh' unsets it, and I just followed suit.
But come to think of it, t1700 has to unset it, because some of its
tests check the index's SHA-1 checksum, and the FSMN extension would
interfere with that, of course.  However, that's not an issue for
t1701, because none of its tests care about the index's checksum, so
unsetting GIT_FSMONITOR_TEST is actually unnecessary here...  unless
it could have other side effects that I'm not aware of.

Re: [PATCH v2 2/3] transport.c: introduce core.alternateRefsCommand

[Taylor Blau]

On Wed, Sep 26, 2018 at 02:38:53PM -0400, Jeff King wrote:
> On Wed, Sep 26, 2018 at 06:39:56AM -0700, Taylor Blau wrote:
>
> > > A perl tangent if you're interested:
> > [...]
> >
> > To be clear, we ought to leave this function as:
> >
> >   extract_haves () {
> > depacketize | perl -lne '/^(\S+) \.have/ and print $1'
> >   }
>
> Yes, I agree. You cannot do the "$@" there because it relies on
> depacketize, which only handles stdin.
>
> > Or are you suggesting that we change it to:
> >
> >   extract_haves () {
> > perl -lne '/^(\S+) \.have/ and print $1'
> >   }
>
> No, sorry. I just used the ".have" snippet as filler text, but I see
> that muddied my meaning considerably. This really was just a tangent for
> the future. What you've written above is the best thing for this case.

I see, and I had assumed that you meant the later, not that including
" .have" was a good way to go forward. So I think that we're in
agreement here.

> > And call it as:
> >
> >   printf "" | git receive-pack fork >actual &&
> >   depacketize actual.packets
> >   extract_haves actual.haves &&
> >
> > Frankly, (and I think that this is what you're getting at in your reply
> > above), I think that the former (e.g., calling 'depacketize()' in
> > 'extract_haves()') is cleaner. This approach leaves us with "actual" and
> > "actual.haves", and obviates the need for another intermediary,
> > "actual.packets".
>
> Yeah. I have no problem with the three-liner you wrote above, but I do
> not see any particular reason for it.

Good. That's the version that I'll send shortly, then.

Thanks,
Taylor

Re: [PATCH] git help: promote 'git help -av'

[Taylor Blau]

On Wed, Sep 26, 2018 at 10:28:31AM -0700, Junio C Hamano wrote:
> Duy Nguyen  writes:
>
> > Here's the patch that adds that external commands and aliases
> > sections. I feel that external commands section is definitely good to
> > have even if we don't replace "help -a". Aliases are more
> > subjective...
>
> I didn't apply this (so I didn't try running it), but a quick
> eyeballing tells me that the listing of external commands in -av
> output done in this patch seems reasonable.
>
> I do not think removing "-v" and making the current "help -a" output
> unavailable is a wise idea, though.

I think that your point about having to react in a split-second in order
to ^C before we open the manual page for a command is a good one. So, I
agree with this.

Thanks,
Taylor

Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree

[Junio C Hamano]

Sam McKelvie  writes:

>>> Subject: Re: [PATCH] submodule: Alllow staged changes for 
>>> get_superproject_working_tree
>> 
>> s/Alllow/allow/;
>> 
>
> Ok, no caps on first letter of subject.

Ah, that, too.  I meant to correct triple ell, though ;-)

When one reviewer says Reviewed-by: but you later found that the
patch was not good enough to deserve a redoing, feel free to redo
the patch and do not add the Reviewed-by: you got for the old one
to your second submission.  The difference between the one that was
reviewed and the one you updated invalidates the stale Reviewed-by:,
essentially.

Some reviewers explicitly state "With this and that nit corrected or
left as-is, the patch is Reviewed-by: me" to tell you that as long
as the difference between the version reviewed and the updated one
is limited within the named issues, you can add Reviewed-by: to your
rerolled patch.

In this case, I think the nits are pretty small and I do not mind
tweaking the version we are discussing on my end, without having you
to send an updated one.

Here is what I'd squash into your commit, with title corrected, if
you are OK with that plan.  In the meantime, I'll keep the following
as a separate patch on top when I merge your fix to 'pu'.

Thanks.

 t/t1500-rev-parse.sh | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index b774cafc5d..01abee533d 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -134,6 +134,7 @@ test_expect_success 'rev-parse --is-shallow-repository in 
non-shallow repo' '
 test_expect_success 'showing the superproject correctly' '
git rev-parse --show-superproject-working-tree >out &&
test_must_be_empty out &&
+
test_create_repo super &&
test_commit -C super test_commit &&
test_create_repo sub &&
@@ -142,20 +143,20 @@ test_expect_success 'showing the superproject correctly' '
echo $(pwd)/super >expect  &&
git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
test_cmp expect out &&
+
test_commit -C super submodule_add &&
git -C super checkout -b branch1 &&
git -C super/dir/sub checkout -b branch1 &&
test_commit -C super/dir/sub branch1_commit &&
git -C super add dir/sub &&
test_commit -C super branch1_commit &&
-   git -C super checkout master &&
-   git -C super checkout -b branch2 &&
-   git -C super/dir/sub checkout master &&
-   git -C super/dir/sub checkout -b branch2 &&
+   git -C super checkout -b branch2 master &&
+   git -C super/dir/sub checkout -b branch2 master &&
test_commit -C super/dir/sub branch2_commit &&
git -C super add dir/sub &&
test_commit -C super branch2_commit &&
test_must_fail git -C super merge branch1 &&
+
git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
test_cmp expect out
 '

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Junio C Hamano]

Jeff King  writes:

> Alternatively, %(HEAD) could return "*" or "+" depending on whether it's
> the current worktree head. That would mildly break an existing format
> like:
>
>   %(if)%(HEAD)%(then) *%(color:green)%(end)%(refname)
>
> since it would start coloring worktree HEADs the same way. It would be
> rewritten as:
>
>   %(if:equals=*)%(HEAD)%(then)...real HEAD...
>   %(else)%(if:equals=+)%(HEAD)%(then)...worktree HEAD...
>   %(else)...regular ref...
>   %(end)%(end)
>
> I think that's perhaps nicer, but I'm not sure we want even such a minor
> regression.

I tend to think it is not worth having to worry about it by changing
the meaning of %(HEAD) marking to save the effort to find a new
token to fill that placeholder.  Your %(worktreeHEAD) is good
enough, I would think.

Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support

[Junio C Hamano]

Josh Steadmon  writes:

> Yes, the version on my desktop sends version=2 when archiving:
>
> ∫ which git
> /usr/bin/git
> ∫ git --version
> git version 2.19.0.605.g01d371f741-goog
> ∫ GIT_TRACE_PACKET=${HOME}/server_trace git daemon \
>   --enable=upload-archive \
>   --base-path=${HOME}/src/bare-repos &
> [1] 258496
> ∫ git archive --remote git://localhost/test-repo.git HEAD >! test.tar
> ∫ grep version ~/server_trace
> 15:31:22.377869 pkt-line.c:80   packet:  git< 
> git-upload-archive /test-repo.git\0host=localhost\0\0version=2\0

Ah, that's truly broken.

Come to think of it, do we need to be using uniform versions across
different endpoints?  The archive request could be at v3 while fetch
request could still be at v2, in which case the design to use a
single protocol.version variable is probably the root cause of the
confusion?  Perhaps like protocol..allow, we would want
protocol..version or something like that (and no
protocol.version) to make it clear that protocol v2 used for
fetching has nothing to do with protocol v1 or v2 or v3 used for
archiving?

Luckily, protocol.version is still marked as experimental so it is
not too bad that we caught the design mistake (if it is one) and can
now correct it before the damage spreads too widely.

Re: [PATCH] read-cache: fix division by zero core-dump

[Ben Peart]

On 9/27/2018 6:24 PM, Ramsay Jones wrote:


commit 225df8a468 ("ieot: add Index Entry Offset Table (IEOT)
extension", 2018-09-26) added a 'DIV_ROUND_UP(entries, ieot_blocks)
expression, where ieot_blocks was set to zero for a single cpu
platform. This caused an SIGFPE and a core dump in practically
every test in the test-suite, until test t4056-diff-order.sh, which
then went into an infinite loop!

Signed-off-by: Ramsay Jones 
---

Hi Ben,

Could you please squash this into the relevant commits on your
'bp/read-cache-parallel' branch. (The first hunk fixes a sparse
warning about using an integer as a NULL pointer).



Absolutely - thanks for the patch.

I don't know how long it's been since I've been on a single core CPU - 
I'm sad for you. ;-)



Thanks!

ATB,
Ramsay Jones

  read-cache.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 6755d58877..40f096f70a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2141,7 +2141,7 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
size_t extension_offset = 0;
  #ifndef NO_PTHREADS
int nr_threads, cpus;
-   struct index_entry_offset_table *ieot = 0;
+   struct index_entry_offset_table *ieot = NULL;
  #endif
  
  	if (istate->initialized)

@@ -2771,7 +2771,7 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
if (ieot_blocks < 1)
ieot_blocks = 1;
cpus = online_cpus();
-   if (ieot_blocks > cpus - 1)
+   if (cpus > 1 && ieot_blocks > cpus - 1)
ieot_blocks = cpus - 1;
} else {
ieot_blocks = nr;

Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs

[Jeff King]

On Thu, Sep 27, 2018 at 04:27:32PM -0700, Jonathan Nieder wrote:

> > There are different opinions on how to document an API properly.
> > Discussion turns out nobody disagrees with documenting new APIs on the
> > function level in the header file and high level concepts in
> > Documentation/technical, so let's state that in the guidelines.
> 
> I disagree with this.  I think we should put all the API docs in the
> header file, like we're already doing in e.g. strbuf.h.

Me too.

I think other high-level concepts that are _not_ APIs (e.g., file
formats, protocol, etc) could go into technical/.

(Though actually, those are the thing that I would not mind at all if
they get formatted into real manpages and shipped to end users. We do
not expect most users to dissect our file-formats, but they could at
least be useful to somebody poking around).

> Do you have a link to where in the discussion this split-docs strategy
> was decided?

I think the purpose of this patch is to spur people towards a decision.
:)

-Peff

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Jeff King]

On Thu, Sep 27, 2018 at 10:35:11PM +0100, Rafael Ascensão wrote:

> git branch has --format, but there's no way (at least to my knowledge)
> to define a value in gitconfig to be used by $git branch.

Oh, you're right. I was thinking of the branch.sort we just added in
v2.19.

I agree that having branch.format (and a matching tag.format) would be
useful.

-Peff

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Jeff King]

On Thu, Sep 27, 2018 at 02:22:49PM -0700, Junio C Hamano wrote:

> The only comment I have is that I strongly suspect we will regret if
> we used an overly bland "worktree" to a rather narrrow "is this ref
> checked out in any worktree?" when we notice we want to learn other
> things that are related to "worktree".  Other than that, very nicely
> done.

Yeah, I should have mentioned that. %(worktree) was just a placeholder.
Perhaps something like %(worktree-HEAD) would make more sense (the idea
is that it is an extension of the existing %(HEAD) placeholder).

Alternatively, %(HEAD) could return "*" or "+" depending on whether it's
the current worktree head. That would mildly break an existing format
like:

  %(if)%(HEAD)%(then) *%(color:green)%(end)%(refname)

since it would start coloring worktree HEADs the same way. It would be
rewritten as:

  %(if:equals=*)%(HEAD)%(then)...real HEAD...
  %(else)%(if:equals=+)%(HEAD)%(then)...worktree HEAD...
  %(else)...regular ref...
  %(end)%(end)

I think that's perhaps nicer, but I'm not sure we want even such a minor
regression.

-Peff

Re: [PATCH v2 1/5] split-index: add tests to demonstrate the racy split index problem

[SZEDER Gábor]

Junio,

On Thu, Sep 27, 2018 at 02:44:30PM +0200, SZEDER Gábor wrote:
> diff --git a/t/t1701-racy-split-index.sh b/t/t1701-racy-split-index.sh
> new file mode 100755
> index 00..ebde418d7e
> --- /dev/null
> +++ b/t/t1701-racy-split-index.sh
> @@ -0,0 +1,218 @@
> +#!/bin/sh
> +
> +# This test can give false success if your machine is sufficiently
> +# slow or all trials happened to happen on second boundaries.
> +
> +test_description='racy split index'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> + # Only split the index when the test explicitly says so.
> + sane_unset GIT_TEST_SPLIT_INDEX GIT_FSMONITOR_TEST &&

Please note that this patch adds another use of the environment
variable GIT_FSMONITOR_TEST, while the topic 'bp/rename-test-env-var'
is about to rename that variable to GIT_TEST_FSMONITOR.

Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree

[Sam McKelvie]

All of your comments seem reasonable; however, since the patch was signed off 
by Stefan it
Is unclear to me whether I should submit another patch or what. I apologize for 
not being
facile with the patching workflow.

> On Sep 27, 2018, at 3:02 PM, Junio C Hamano  wrote:
> 
> Sam McKelvie  writes:
> 
>> Subject: Re: [PATCH] submodule: Alllow staged changes for 
>> get_superproject_working_tree
> 
> s/Alllow/allow/;
> 

Ok, no caps on first letter of subject.

>> Invoking 'git rev-parse --show-superproject-working-tree' exits with
>> 
>>"fatal: BUG: returned path string doesn't match cwd?"
>> 
>> when the superproject has an unmerged entry for the current submodule,
>> instead of displaying the superproject's working tree.
>> 
>> The problem is due to the fact that when a merge of the submodule reference
>> is in progress, "git ls-files --stage —full-name ”
>> returns three seperate entries for the submodule (one for each stage) rather
>> than a single entry; e.g.,
>> 
>> $ git ls-files --stage --full-name submodule-child-test
>> 16 dbbd2766fa330fa741ea59bb38689fcc2d283ac5 1   submodule-child-test
>> 16 f174d1dbfe863a59692c3bdae730a36f2a788c51 2   submodule-child-test
>> 16 e6178f3a58b958543952e12824aa2106d560f21d 3   submodule-child-test
>> 
>> The code in get_superproject_working_tree() expected exactly one entry to
>> be returned; this patch makes it use the first entry if multiple entries
>> are returned.
>> 
>> Test t1500-rev-parse is extended to cover this case.
>> 
>> Signed-off-by: Sam McKelvie 
>> ---
>> submodule.c  |  2 +-
>> t/t1500-rev-parse.sh | 17 -
>> 2 files changed, 17 insertions(+), 2 deletions(-)
>> 
>> diff --git a/submodule.c b/submodule.c
>> index 33de6ee5f..5b9d5ad7e 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1885,7 +1885,7 @@ const char *get_superproject_working_tree(void)
>>   * We're only interested in the name after the tab.
>>   */
>>  super_sub = strchr(sb.buf, '\t') + 1;
>> -super_sub_len = sb.buf + sb.len - super_sub - 1;
>> +super_sub_len = strlen(super_sub);
> 
> As we are reading from "ls-files -z -s", we know that the name is
> terminated with NUL, so we can just use strlen().  Good.
>> 
>>  if (super_sub_len > cwd_len ||
>>  strcmp([cwd_len - super_sub_len], super_sub))
>> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
>> index 5c715fe2c..b774cafc5 100755
>> --- a/t/t1500-rev-parse.sh
>> +++ b/t/t1500-rev-parse.sh
>> @@ -134,7 +134,6 @@ test_expect_success 'rev-parse --is-shallow-repository 
>> in non-shallow repo' '
>> test_expect_success 'showing the superproject correctly' '
>>  git rev-parse --show-superproject-working-tree >out &&
>>  test_must_be_empty out &&
>> -
> 
> I have a feeling that this break made the series of tests in this
> block easier to follow.  Shouldn't we be moving in the other
> direction, namely …
> 
That’s fair.


>>  test_create_repo super &&
>>  test_commit -C super test_commit &&
>>  test_create_repo sub &&
>> @@ -142,6 +141,22 @@ test_expect_success 'showing the superproject 
>> correctly' '
>>  git -C super submodule add ../sub dir/sub &&
>>  echo $(pwd)/super >expect  &&
>>  git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
>> +test_cmp expect out &&
> 
> Here is an end of one subtest, deserves to have a break like the above.

OK

> 
>> +test_commit -C super submodule_add &&
>> +git -C super checkout -b branch1 &&
>> +git -C super/dir/sub checkout -b branch1 &&
>> +test_commit -C super/dir/sub branch1_commit &&
>> +git -C super add dir/sub &&
>> +test_commit -C super branch1_commit &&
>> +git -C super checkout master &&
>> +git -C super checkout -b branch2 &&
>> +git -C super/dir/sub checkout master &&
>> +git -C super/dir/sub checkout -b branch2 &&
>> +test_commit -C super/dir/sub branch2_commit &&
>> +git -C super add dir/sub &&
>> +test_commit -C super branch2_commit &&
>> +test_must_fail git -C super merge branch1 &&
> 
> and all of the above is just a set-up for another subtest, so a
> solid block of text like we see in the above is good.
> 
>   Side note: there are a few of
> 
>   git -C $there checkout $onebranch &&
>   git -C $there checkout -b $anotherbranch &&
> 
>   as recurring pattern.  Shouldn't they be more like a single
>   liner
> 
>   git -C $there checkout -b $anotherbranch $onebranch &&
> 
>   ?  It wasn't clear if the split was an attempt to hide some
>   breakage (e.g. "checkout -b B A" did not work but "checkout
>   A && checkout -b B" did) or just being verbose because the
>   author is not used to "checkout -b B A" form.

You’re right, the two forms are equivalent and the single-line version is 
simpler.

> 
>> +git -C super/dir/sub rev-parse 

Re: [PATCH v6 4/7] config: add new index.threads config setting

[SZEDER Gábor]

On Wed, Sep 26, 2018 at 03:54:39PM -0400, Ben Peart wrote:
> Add support for a new index.threads config setting which will be used to
> control the threading code in do_read_index().  A value of 0 will tell the
> index code to automatically determine the correct number of threads to use.
> A value of 1 will make the code single threaded.  A value greater than 1
> will set the maximum number of threads to use.
> 
> For testing purposes, this setting can be overwritten by setting the
> GIT_TEST_INDEX_THREADS= environment variable to a value greater than 0.
> 
> Signed-off-by: Ben Peart 
> ---

> diff --git a/t/README b/t/README
> index aa33ac4f26..0fcecf4500 100644
> --- a/t/README
> +++ b/t/README
> @@ -332,6 +332,11 @@ This is used to allow tests 1, 4-9 in 
> t1700-split-index.sh to succeed
>  as they currently hard code SHA values for the index which are no longer
>  valid due to the addition of the EOIE extension.
>  
> +GIT_TEST_INDEX_THREADS= enables exercising the multi-threaded loading
> +of the index for the whole test suite by bypassing the default number of
> +cache entries and thread minimums. Settting this to 1 will make the

s/ttt/tt/

> +index loading single threaded.
> +
>  Naming Tests
>  
>  
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 1f168378c8..ab205954cf 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -8,6 +8,7 @@ test_description='split index mode tests'
>  sane_unset GIT_TEST_SPLIT_INDEX
>  sane_unset GIT_FSMONITOR_TEST
>  GIT_TEST_DISABLE_EOIE=true; export GIT_TEST_DISABLE_EOIE
> +GIT_TEST_INDEX_THREADS=1; export GIT_TEST_INDEX_THREADS

Why does multithreading have to be disabled in this test?

>  test_expect_success 'enable split index' '
>   git config splitIndex.maxPercentChange 100 &&
> -- 
> 2.18.0.windows.1
> 

Re: [PATCH v6 3/7] eoie: add End of Index Entry (EOIE) extension

[SZEDER Gábor]

On Wed, Sep 26, 2018 at 03:54:38PM -0400, Ben Peart wrote:
> The End of Index Entry (EOIE) is used to locate the end of the variable

Nit: perhaps start with: 

  The End of Index Entry (EOIE) optional extension can be used to ...

to make it clearer for those who don't immediately realize the
significance of the upper case 'E' in the extension's signature.

> length index entries and the beginning of the extensions. Code can take
> advantage of this to quickly locate the index extensions without having
> to parse through all of the index entries.
> 
> Because it must be able to be loaded before the variable length cache
> entries and other index extensions, this extension must be written last.
> The signature for this extension is { 'E', 'O', 'I', 'E' }.
> 
> The extension consists of:
> 
> - 32-bit offset to the end of the index entries
> 
> - 160-bit SHA-1 over the extension types and their sizes (but not
> their contents).  E.g. if we have "TREE" extension that is N-bytes
> long, "REUC" extension that is M-bytes long, followed by "EOIE",
> then the hash would be:
> 
> SHA-1("TREE" +  +
>   "REUC" + )
> 
> Signed-off-by: Ben Peart 
> ---
>  Documentation/technical/index-format.txt |  23 
>  read-cache.c | 151 +--
>  t/README |   5 +
>  t/t1700-split-index.sh   |   1 +
>  4 files changed, 172 insertions(+), 8 deletions(-)
> 

> diff --git a/t/README b/t/README
> index 3ea6c85460..aa33ac4f26 100644
> --- a/t/README
> +++ b/t/README
> @@ -327,6 +327,11 @@ GIT_TEST_COMMIT_GRAPH=, when true, forces the 
> commit-graph to
>  be written after every 'git commit' command, and overrides the
>  'core.commitGraph' setting to true.
>  
> +GIT_TEST_DISABLE_EOIE= disables writing the EOIE extension.
> +This is used to allow tests 1, 4-9 in t1700-split-index.sh to succeed
> +as they currently hard code SHA values for the index which are no longer
> +valid due to the addition of the EOIE extension.

Is this extension enabled by default?  The commit message doesn't
explicitly say so, but I don't see any way to turn it on or off, while
there is this new GIT_TEST environment variable to disable it for one
particular test, so it seems so.  If that's indeed the case, then
wouldn't it be better to update those hard-coded SHA1 values in t1700
instead?

>  Naming Tests
>  
>  
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index be22398a85..1f168378c8 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -7,6 +7,7 @@ test_description='split index mode tests'
>  # We need total control of index splitting here
>  sane_unset GIT_TEST_SPLIT_INDEX
>  sane_unset GIT_FSMONITOR_TEST
> +GIT_TEST_DISABLE_EOIE=true; export GIT_TEST_DISABLE_EOIE
>  
>  test_expect_success 'enable split index' '
>   git config splitIndex.maxPercentChange 100 &&
> -- 
> 2.18.0.windows.1
> 

Re: [PATCH] negotiator/skipping: parse commit before queueing

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 27 2018, Jonathan Tan wrote:

> The skipping negotiator pushes entries onto the priority queue without
> ensuring that the commit is parsed, resulting in the entry ending up in
> the wrong position due to a lack of commit time. Fix this by parsing the
> commit whenever we push an entry onto the priority queue.
>
> Signed-off-by: Jonathan Tan 
> ---

Thanks for the prompt fix!

> This was noticed by Aevar in [1]. With this fix, 163 "have" lines are
> produced instead of the 14002 reported in [1].
>
> I have included a test to demonstrate the issue, but I'm not sure if
> it's worth including it in the source tree.

The test fails before the patch, and passes after, and tests that we do
the right thing here. Seems best to include such tests whenever we can.

Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

> There are different opinions on how to document an API properly.
> Discussion turns out nobody disagrees with documenting new APIs on the
> function level in the header file and high level concepts in
> Documentation/technical, so let's state that in the guidelines.

I disagree with this.  I think we should put all the API docs in the
header file, like we're already doing in e.g. strbuf.h.

Do you have a link to where in the discussion this split-docs strategy
was decided?

Thanks,
Jonathan

[PATCH] negotiator/skipping: parse commit before queueing

[Jonathan Tan]

The skipping negotiator pushes entries onto the priority queue without
ensuring that the commit is parsed, resulting in the entry ending up in
the wrong position due to a lack of commit time. Fix this by parsing the
commit whenever we push an entry onto the priority queue.

Signed-off-by: Jonathan Tan 
---
This was noticed by Aevar in [1]. With this fix, 163 "have" lines are
produced instead of the 14002 reported in [1].

I have included a test to demonstrate the issue, but I'm not sure if
it's worth including it in the source tree.

[1] https://public-inbox.org/git/87o9ciisg6@evledraar.gmail.com/
---
 negotiator/skipping.c|  2 +-
 t/t5552-skipping-fetch-negotiator.sh | 22 ++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/negotiator/skipping.c b/negotiator/skipping.c
index dffbc76c49..9ce0555840 100644
--- a/negotiator/skipping.c
+++ b/negotiator/skipping.c
@@ -64,6 +64,7 @@ static struct entry *rev_list_push(struct data *data, struct 
commit *commit, int
 
entry = xcalloc(1, sizeof(*entry));
entry->commit = commit;
+   parse_commit(commit);
prio_queue_put(>rev_list, entry);
 
if (!(mark & COMMON))
@@ -177,7 +178,6 @@ static const struct object_id *get_rev(struct data *data)
if (!(commit->object.flags & COMMON) && !entry->ttl)
to_send = commit;
 
-   parse_commit(commit);
for (p = commit->parents; p; p = p->next)
parent_pushed |= push_parent(data, entry, p->item);
 
diff --git a/t/t5552-skipping-fetch-negotiator.sh 
b/t/t5552-skipping-fetch-negotiator.sh
index 30857b84a8..f2f938e54e 100755
--- a/t/t5552-skipping-fetch-negotiator.sh
+++ b/t/t5552-skipping-fetch-negotiator.sh
@@ -83,6 +83,28 @@ test_expect_success 'unknown fetch.negotiationAlgorithm 
values error out' '
test_i18ngrep ! "unknown fetch negotiation algorithm" err
 '
 
+test_expect_success 'works in absence of tags' '
+   rm -rf server client trace &&
+   git init server &&
+   test_commit -C server to_fetch &&
+
+   git init client &&
+   for i in $(test_seq 11)
+   do
+   test_commit -C client c$i
+   done &&
+   git -C client tag middle c6 &&
+   for i in $(test_seq 11)
+   do
+   git -C client tag -d c$i
+   done &&
+
+   test_config -C client fetch.negotiationalgorithm skipping &&
+   trace_fetch client "$(pwd)/server" &&
+   have_sent HEAD HEAD~2 HEAD~5 HEAD~10 &&
+   have_not_sent HEAD~1 HEAD~3 HEAD~4 HEAD~6 HEAD~7 HEAD~8 HEAD~9
+'
+
 test_expect_success 'when two skips collide, favor the larger one' '
rm -rf server client trace &&
git init server &&
-- 
2.19.0.605.g01d371f741-goog

Re: Fixing constant preference prompts during tests

[brian m. carlson]

On Thu, Sep 27, 2018 at 12:42:48AM +, Tacitus Aedifex wrote:
> On Wed, Sep 26, 2018 at 02:48:49PM -0700, Junio C Hamano wrote:
> > We do not want your choice of gpg.program or what kind of
> > trust you have personally recorded in your ~/.gnupg/ affect how gpg
> > invoked inside our tests work.
> 
> This makes sense to me now. I get what you are saying. The gpg binary
> installed on my system is fairly new:
> 
> gpg --version
> gpg (GnuPG) 2.2.8
> libgcrypt 1.8.3
> Copyright (C) 2018 Free Software Foundation, Inc.
> License GPLv3+: GNU GPL version 3 or later 
> This is free software: you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.
> 
> Home: /home/user/.gnupg
> Supported algorithms:
> Pubkey: RSA, ELG, DSA, ECDH, ECDSA, EDDSA
> Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
>CAMELLIA128, CAMELLIA192, CAMELLIA256
> Hash: SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
> Compression: Uncompressed

I'm using GnuPG 2.2.10 from Debian unstable on my system and I don't see
this issue.

> I'm not sure if that has anything to do with it. I'm going to have to
> investigate further to figure out what is being executed and with what
> parameters that leads to the prerefences prompt. While working with GPG on
> another project I noticed that GPG doesn't like to work with keyrings other
> than the default ones. I tried a bunch of different combinations of
> --no-default-keyrings, --homedir, --default-key, etc to try to get GPG to
> never touch ~/.gnupg and I couldn't figure it out. It would always re-create
> ~/.gnupg and default keyrings and even read gpg.conf when explicitly told
> not to. I suspect that is what is going on here.

It might be worth checking to see if you have a gpg binary or script in
your PATH that isn't the one you expect.  That's broken things for me in
the past.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [RFC PATCH] travis-ci: work around quarantine error during Perforce install on macOS

[SZEDER Gábor]

On Fri, Sep 07, 2018 at 05:20:02AM +0200, SZEDER Gábor wrote:
> Homebrew recently enabled the quarantine feature, which breaks a lot
> of things [1], including installing Perforce in our macOS build jobs
> on Travis CI [2], breaking all those builds in the last couple of
> days.
 
> Note that one option homebrew/cask devs are considering is to roll
> back quarantine altogether [3].

> Finally, if we wait long enough, then this whole issue might just
> solve itself, if the homebrew devs revert this quarantine stuff, and
> our current 'brew install' command might suddenly work again.

Good, we waited long enough, and this issue apparently did indeed just
solve itself :)

Re: [PATCH 0/2] negotiator: improve recent behavior + docs

[Jonathan Tan]

> I get:
> 
> warning: Ignoring --negotiation-tip because the protocol does not support 
> it.

When I implemented --negotiation-tip, I only implemented it for
protocols that support connect or stateless-connect, because
implementing it fully would have required expanding the protocol helper
functionality. For reference, the commit is 3390e42adb ("fetch-pack:
support negotiation tip whitelist", 2018-07-03).

So HTTPS wouldn't work unless you were using protocol v2.

> So that seems like another bug, and as an aside, a "skipping"
> implementation that sends ~1/4 of the commits in the repo seems way less
> aggressive than it should be. I was expecting something that would
> gradually "ramp up" from the tips. Where say starting at master/next/pu
> we present every 100th commit as a "have" until the 1000th commit, then
> every 1000 commits until 10k and quickly after that step up the size
> rapidly.

I reproduced using your commands, and yes, there is a bug - I'll take a
look.

Re: [PATCH] Add an EditorConfig file

[brian m. carlson]

On Mon, Sep 24, 2018 at 01:05:23PM -0700, Junio C Hamano wrote:
> > Would it be helpful if I sent a script that ran during CI to ensure they
> > stayed in sync for the couple places where they overlap?  I'm happy to
> > do so if you think it would be useful.
> 
> It may even be an overkill.
> 
> A comment addressed to those who edit one file to look at the other
> file on both files would be sufficient, I suspect.

Sure.  Let me reroll with that change.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support

[Josh Steadmon]

On 2018.09.27 15:20, Junio C Hamano wrote:
> Jonathan Nieder  writes:
> 
> >  1. Clients sending version=2 when they do not, in fact, speak protocol
> > v2 for a service is a (serious) bug.  (Separately from this
> > series) we should fix it.
> >
> >  2. That bug is already in the wild, alas.  Fortunately the semantics of
> > GIT_PROTOCOL as a list of key/value pairs is well defined.  So we
> > have choices of (a) bump version to version=3 (b) pass another
> > value 'version=2:yesreallyversion=2' (c) etc.
> >
> >  3. This is likely to affect push, too.
> 
> Do you mean that existing "git push", "git fetch" and "git archive"
> sends version=2 even when they are not capable of speaking protocol
> v2?  I thought that "git archive [--remote]" was left outside of the
> protocol update (that was the reason why the earlier attempt took a
> hacky route of "shallow clone followed by local archive"), so there
> is no "git archive" in the wild that can even say "version=$n"
> (which requires you to be at least version=1)?

Yes, the version on my desktop sends version=2 when archiving:

∫ which git
/usr/bin/git
∫ git --version
git version 2.19.0.605.g01d371f741-goog
∫ GIT_TRACE_PACKET=${HOME}/server_trace git daemon \
  --enable=upload-archive \
  --base-path=${HOME}/src/bare-repos &
[1] 258496
∫ git archive --remote git://localhost/test-repo.git HEAD >! test.tar
∫ grep version ~/server_trace
15:31:22.377869 pkt-line.c:80   packet:  git< 
git-upload-archive /test-repo.git\0host=localhost\0\0version=2\0

--skip-worktree and operations like checkout / stash /etc.

[Raman Gupta]

The comand `update-index --skip-worktree` seems to be an ideal way to
tell git to locally ignore some modified files. However, this seems
not to play well with very common commands like `checkout` and
`stash`.

$ git checkout other-branch
error: Your local changes to the following files would be overwritten
by checkout:
path/to/ignored/file
Please commit your changes or stash them before you switch branches.
Aborting

Ok, well lets try stashing:

$ git stash save
No local changes to save

Ok, lets try a checkout with a merge:

$ git checkout -m other-branch
error: Entry 'path/to/ignored/file' not uptodate. Cannot merge.

Ok, lets force this sucker:

$ git checkout -f other-branch
error: Entry 'path/to/ignored/file' not uptodate. Cannot merge.

Ok, at this point I'm wondering, do I really need to
--no-skip-worktree all the ignored files, do my `checkout -m`, and
then ignore them again? Umm, no, that ain't gonna work.

I'd love for git to just check if my worktree-skipped changes will
merge cleanly into the target branch, and if they do so, go ahead and
do that merge (with perhaps a notification printed to the console) and
keep the skip worktree status. If the merge ends up with a conflict,
then feel free to no-worktree-skip it and show me merge conflicts.

Regards,
Raman

Re: [PATCH] FYI / RFC: submodules: introduce repo-like workflow

[Jonathan Nieder]

(dropping cc-s to my internal address that I don't use on this list
 and to git-c...@google.com which bounces)
Hi,

Stefan Beller wrote:

> Internally we have rolled out this as an experiment for
> "submodules replacing the repo tool[1]". The repo tool is described as:
>
> Repo unifies Git repositories when necessary, performs uploads to the
> Gerrit revision control system, and automates parts of the Android
> development workflow. Repo is not meant to replace Git, only to make
> it easier to work with Git in the context of Android. The repo command
> is an executable Python script that you can put anywhere in your path.
[...]
> Submodules can also be understood as unifying Git repositories, even more,
> in the superproject the submodules have relationships between their
> versions, which the repo tool only provides in releases.
>
> The repo tool does not provide these relationships between versions, but
> all the repositories (in case of Android think of ~1000 git repositories)
> are put in their place without depending on each other.
>
> This comes with a couple of advantages and disadvantages:

Thanks for describing this background.

[...]
> This changes the following commands in the superproject:
>
>   checkout -B/-b create branches in subs, too
>   checkout (-f): update branch in submodule (create if needed) and check
>  it out; Pass the argument down literally if it is a branch
>  name (e.g. "checkout -f master" will run a
> "checkout -f master" in the submodule as well)
>   clone: see checkout
>   reset --hard: see checkout

As you mentioned, I've been using this submodule.repoLike=true mode
for my own use for a while.  You did a nice job of explaining on how
it fits into a Gerrit-driven workflow; I'd like to add that I find it
pleasant in non-Gerrit-driven contexts as well.

The primary difference from repoLike=false is that this makes the
normal state to have branches checked out in submodules.  For example,
if I run

git checkout --recurse-submodules -B master origin/master

then this will create and check out a "master" branch in all
submodules instead of only in the superproject.  This helps avoid some
issues in Git's submodule handling where submodule commits can be
pruned if they have not been checked out in a while because there is
no ref pointing to them.

Some next steps:

- now that we have a repository object, some of the implementation can
  be simplified and made more robust.  I expect that will also make
  these patches easier to review

- also in the direction of reviewability, at that point we may want to
  split this into multiple patches

- gitsubmodules.txt and config.txt should describe the new option, to
  help new users understand what this new repoLike workflow does

- there are some edge cases in the UX that get... messy that I should
  describe in another message

All that said, thanks for sending this out, and I'd be happy to hear
from any interested people --- feedback from anyone adventurous enough
to try this out would be very welcome.

Happy hacking,
Jonathan

[PATCH] read-cache: fix division by zero core-dump

[Ramsay Jones]

commit 225df8a468 ("ieot: add Index Entry Offset Table (IEOT)
extension", 2018-09-26) added a 'DIV_ROUND_UP(entries, ieot_blocks)
expression, where ieot_blocks was set to zero for a single cpu
platform. This caused an SIGFPE and a core dump in practically
every test in the test-suite, until test t4056-diff-order.sh, which
then went into an infinite loop!

Signed-off-by: Ramsay Jones 
---

Hi Ben,

Could you please squash this into the relevant commits on your
'bp/read-cache-parallel' branch. (The first hunk fixes a sparse
warning about using an integer as a NULL pointer).

Thanks!

ATB,
Ramsay Jones

 read-cache.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 6755d58877..40f096f70a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2141,7 +2141,7 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
size_t extension_offset = 0;
 #ifndef NO_PTHREADS
int nr_threads, cpus;
-   struct index_entry_offset_table *ieot = 0;
+   struct index_entry_offset_table *ieot = NULL;
 #endif
 
if (istate->initialized)
@@ -2771,7 +2771,7 @@ static int do_write_index(struct index_state *istate, 
struct tempfile *tempfile,
if (ieot_blocks < 1)
ieot_blocks = 1;
cpus = online_cpus();
-   if (ieot_blocks > cpus - 1)
+   if (cpus > 1 && ieot_blocks > cpus - 1)
ieot_blocks = cpus - 1;
} else {
ieot_blocks = nr;
-- 
2.19.0

Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 27 2018, Junio C Hamano wrote:

> Stefan Beller  writes:
>
>> There are different opinions on how to document an API properly.
>> Discussion turns out nobody disagrees with documenting new APIs on the
>> function level in the header file and high level concepts in
>
> Looks conditionally good ;-) Thanks for keeping the ball rolling.
>
> I didn't get an impression that "next to implementation" vs "in
> header to force people write clearly" was totally settled.  I'd wait
> until Ævar says something on this ;-)

I think this patch is good and should go in. Having it be consistent is
a good thing, and we're drifting more in the *.h direction than *.txt.

The "next to implementation" suggestion was in the context of what the
perl project does, to get good use out of that we'd a) need to move all
the *.h docs and *.txt to *.c b) have something to slurp up those docs
so they're formatted. I'm not about to submit any patches for that.

>> Documentation/technical, so let's state that in the guidelines.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>
>>  This is how I would approach the documentation patch.
>>
>>  Thanks,
>>  Stefan
>>
>>  Documentation/CodingGuidelines | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
>> index 48aa4edfbd..15bfb8bbb8 100644
>> --- a/Documentation/CodingGuidelines
>> +++ b/Documentation/CodingGuidelines
>> @@ -358,7 +358,10 @@ For C programs:
>> string_list for sorted string lists, a hash map (mapping struct
>> objects) named "struct decorate", amongst other things.
>>
>> - - When you come up with an API, document it.
>> + - When you come up with an API, document the functions in the header
>> +   and highlevel concepts in Documentation/technical/. Note that this
>> +   directory still contains function level documentation as historically
>> +   everything was documented there.
>>
>>   - The first #include in C files, except in platform specific compat/
>> implementations, must be either "git-compat-util.h", "cache.h" or

Re: [PATCH v2 0/4] Add proto v2 archive command with HTTP support

[Junio C Hamano]

Jonathan Nieder  writes:

>  1. Clients sending version=2 when they do not, in fact, speak protocol
> v2 for a service is a (serious) bug.  (Separately from this
> series) we should fix it.
>
>  2. That bug is already in the wild, alas.  Fortunately the semantics of
> GIT_PROTOCOL as a list of key/value pairs is well defined.  So we
> have choices of (a) bump version to version=3 (b) pass another
> value 'version=2:yesreallyversion=2' (c) etc.
>
>  3. This is likely to affect push, too.

Do you mean that existing "git push", "git fetch" and "git archive"
sends version=2 even when they are not capable of speaking protocol
v2?  I thought that "git archive [--remote]" was left outside of the
protocol update (that was the reason why the earlier attempt took a
hacky route of "shallow clone followed by local archive"), so there
is no "git archive" in the wild that can even say "version=$n"
(which requires you to be at least version=1)?

[PATCH] FYI / RFC: submodules: introduce repo-like workflow

[Stefan Beller]

Internally we have rolled out this as an experiment for
"submodules replacing the repo tool[1]". The repo tool is described as:

Repo unifies Git repositories when necessary, performs uploads to the
Gerrit revision control system, and automates parts of the Android
development workflow. Repo is not meant to replace Git, only to make
it easier to work with Git in the context of Android. The repo command
is an executable Python script that you can put anywhere in your path.

In working with the Android source files, you use Repo for
across-network operations. For example, with a single Repo command you
can download files from multiple repositories into your local working
directory.

In most situations, you can use Git instead of Repo, or mix Repo and
Git commands to form complex commands.

[1] https://source.android.com/setup/develop/

Submodules can also be understood as unifying Git repositories, even more,
in the superproject the submodules have relationships between their
versions, which the repo tool only provides in releases.

The repo tool does not provide these relationships between versions, but
all the repositories (in case of Android think of ~1000 git repositories)
are put in their place without depending on each other.

This comes with a couple of advantages and disadvantages:

* Many users are familiar with Git, but not submodules. Each repository
  can be used independently with Git and there is no need to update the
  superproject or the repo manifest for a change in a repository.
* It is easy to work with repositories with no version-control-dependencies
  if there are dependencies in the code. In case of Android the
  repositories are bound at natural boundaries. For example the linux
  kernel is one repository, as then upstream work is made easy for this
  repository. So it is desirable to keep an easy-as-repo workflow.
* Fetching changes ("repo sync") needs to fetch all repositories, as there
  is no central place that tracks what has changed. In a superproject
  git fetch can determine which submodules need fetching.  In Androids
  case the daily change is only in a few repositories (think 10s), so
  migrating to a superproject would save an order of magnitude in fetch
  traffic for daily updates of developers.
* Sometimes when the dependencies are not on a clear repository boundary
  one would like to have git-bisect available across the different
  repositories, which repo cannot provide due to its design.

Internally we have the Gerrit as a central point, where the source of
truth is found for a given repository.

This patch adds a new mode to submodule handling, where the superproject
controls the existence of the submodule (just as current submodule
handling), but the submodule HEAD is not detached, but following the same
branch name as the superproject.

Current situation visualized:

  superproject
  HEAD -> "" -> OID
  |
  submodule   v
  HEAD > OID

The OID in the submodule is controlled via the HEAD in the submodule that
is set accordingly to the gitlink in the superproject. Confusion can arise
when the (detached) HEAD in the submodule doesn't match the superprojects
gitlink.

This patch visualized:

  superproject
  HEAD -> "" -> OID
 |
  submodule  v
  HEAD -> "" -> OID

As there is a central point of truth in our setup (our Gerrit installation)
which keeps the superproject and the submodule branches in sync, this
ought to look the same for the user; removing the "detached HEAD" in the
submodule. git-status will still notice if there is an OID mismatch between
the gitlink and the submodules branch, but that is a race condition and
should be caught by Gerrit.

This changes the following commands in the superproject:

  checkout -B/-b create branches in subs, too
  checkout (-f): update branch in submodule (create if needed) and check
 it out; Pass the argument down literally if it is a branch
 name (e.g. "checkout -f master" will run a
"checkout -f master" in the submodule as well)
  clone: see checkout
  reset --hard: see checkout

Change-Id: I69b361e5bd9d57226a0976fd36968cf9aeb52ae0
Signed-off-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 Documentation/git-checkout.txt | 13 +++--
 Makefile   |  1 +
 branch.c   | 84 +
 builtin.h  |  1 +
 builtin/branch.c   | 35 ++
 builtin/checkout.c | 76 --
 builtin/clone.c| 12 -
 builtin/reset.c| 41 ++--
 entry.c| 43 +
 git-submodule.sh   | 24 +-
 git.c  |  1 +
 submodule-move-head.c  | 81 
 submodule-move-head.h  | 

[PATCH] fetch: fix compilation warning

[Ramsay Jones]

commit 440fc7c0729 ("fetch: replace string-list used as a look-up
table with a hashmap", 2018-09-25) renamed a string-list variable
(while adding a hashmap of the same name) and forgot to rename the
string-list variable in a call to string_list_clear().

Signed-off-by: Ramsay Jones 
---

Hi Junio,

You probably already know, but I had to add this on top of the 'pu'
branch to get a clean compile tonight (your 'jc/war-on-string-list'
branch).

Thanks!

ATB,
Ramsay Jones

 builtin/fetch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0a71953bc5..aea2e10364 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -391,7 +391,7 @@ static void find_non_local_tags(const struct ref *refs,
}
}
hashmap_free(_refs, 1);
-   string_list_clear(_refs, 0);
+   string_list_clear(_refs_list, 0);
 }
 
 static struct ref *get_ref_map(struct remote *remote,
-- 
2.19.0

Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree

[Junio C Hamano]

Sam McKelvie  writes:

> Subject: Re: [PATCH] submodule: Alllow staged changes for 
> get_superproject_working_tree

s/Alllow/allow/;

> Invoking 'git rev-parse --show-superproject-working-tree' exits with
>
> "fatal: BUG: returned path string doesn't match cwd?"
>
> when the superproject has an unmerged entry for the current submodule,
> instead of displaying the superproject's working tree.
>
> The problem is due to the fact that when a merge of the submodule reference
> is in progress, "git ls-files --stage —full-name ”
> returns three seperate entries for the submodule (one for each stage) rather
> than a single entry; e.g.,
>
> $ git ls-files --stage --full-name submodule-child-test
> 16 dbbd2766fa330fa741ea59bb38689fcc2d283ac5 1   submodule-child-test
> 16 f174d1dbfe863a59692c3bdae730a36f2a788c51 2   submodule-child-test
> 16 e6178f3a58b958543952e12824aa2106d560f21d 3   submodule-child-test
>
> The code in get_superproject_working_tree() expected exactly one entry to
> be returned; this patch makes it use the first entry if multiple entries
> are returned.
>
> Test t1500-rev-parse is extended to cover this case.
>
> Signed-off-by: Sam McKelvie 
> ---
>  submodule.c  |  2 +-
>  t/t1500-rev-parse.sh | 17 -
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 33de6ee5f..5b9d5ad7e 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1885,7 +1885,7 @@ const char *get_superproject_working_tree(void)
>* We're only interested in the name after the tab.
>*/
>   super_sub = strchr(sb.buf, '\t') + 1;
> - super_sub_len = sb.buf + sb.len - super_sub - 1;
> + super_sub_len = strlen(super_sub);

As we are reading from "ls-files -z -s", we know that the name is
terminated with NUL, so we can just use strlen().  Good.
>  
>   if (super_sub_len > cwd_len ||
>   strcmp([cwd_len - super_sub_len], super_sub))
> diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
> index 5c715fe2c..b774cafc5 100755
> --- a/t/t1500-rev-parse.sh
> +++ b/t/t1500-rev-parse.sh
> @@ -134,7 +134,6 @@ test_expect_success 'rev-parse --is-shallow-repository in 
> non-shallow repo' '
>  test_expect_success 'showing the superproject correctly' '
>   git rev-parse --show-superproject-working-tree >out &&
>   test_must_be_empty out &&
> -

I have a feeling that this break made the series of tests in this
block easier to follow.  Shouldn't we be moving in the other
direction, namely ...

>   test_create_repo super &&
>   test_commit -C super test_commit &&
>   test_create_repo sub &&
> @@ -142,6 +141,22 @@ test_expect_success 'showing the superproject correctly' 
> '
>   git -C super submodule add ../sub dir/sub &&
>   echo $(pwd)/super >expect  &&
>   git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
> + test_cmp expect out &&

Here is an end of one subtest, deserves to have a break like the above.

> + test_commit -C super submodule_add &&
> + git -C super checkout -b branch1 &&
> + git -C super/dir/sub checkout -b branch1 &&
> + test_commit -C super/dir/sub branch1_commit &&
> + git -C super add dir/sub &&
> + test_commit -C super branch1_commit &&
> + git -C super checkout master &&
> + git -C super checkout -b branch2 &&
> + git -C super/dir/sub checkout master &&
> + git -C super/dir/sub checkout -b branch2 &&
> + test_commit -C super/dir/sub branch2_commit &&
> + git -C super add dir/sub &&
> + test_commit -C super branch2_commit &&
> + test_must_fail git -C super merge branch1 &&

and all of the above is just a set-up for another subtest, so a
solid block of text like we see in the above is good.

Side note: there are a few of

git -C $there checkout $onebranch &&
git -C $there checkout -b $anotherbranch &&

as recurring pattern.  Shouldn't they be more like a single
liner

git -C $there checkout -b $anotherbranch $onebranch &&

?  It wasn't clear if the split was an attempt to hide some
breakage (e.g. "checkout -b B A" did not work but "checkout
A && checkout -b B" did) or just being verbose because the
author is not used to "checkout -b B A" form.

> + git -C super/dir/sub rev-parse --show-superproject-working-tree >out &&
>   test_cmp expect out
>  '

Thanks.

[GSoC][PATCH v8 10/20] t3404: todo list with commented-out commands only aborts

[Alban Gruin]

If the todo list generated by `--make-script` is empty,
complete_action() writes a noop, but if it has only commented-out
commands, it will abort with the message "Nothing to do", and does not
launch the editor.  This adds a new test to ensure that
complete_action() behaves this way.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 t/t3404-rebase-interactive.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ff89b6341a..a7fc3cd5be 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -75,6 +75,16 @@ test_expect_success 'rebase --keep-empty' '
test_line_count = 6 actual
 '
 
+cat > expect &1 &&
+   test_i18ncmp expect actual
+'
+
 test_expect_success 'rebase -i with the exec command' '
git checkout master &&
(
-- 
2.19.0

[GSoC][PATCH v8 18/20] rebase--interactive2: rewrite the submodes of interactive rebase in C

[Alban Gruin]

This rewrites the submodes of interactive rebase (`--continue`,
`--skip`, `--edit-todo`, and `--show-current-patch`) in C.

git-rebase.sh is then modified to call directly git-rebase--interactive2
instead of git-rebase--interactive.sh.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--interactive2.c | 51 ++
 git-rebase.sh  | 45 +++---
 2 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/builtin/rebase--interactive2.c b/builtin/rebase--interactive2.c
index 45336073a8..aaf13c9621 100644
--- a/builtin/rebase--interactive2.c
+++ b/builtin/rebase--interactive2.c
@@ -134,11 +134,14 @@ int cmd_rebase__interactive(int argc, const char **argv, 
const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
-   int abbreviate_commands = 0, rebase_cousins = -1;
+   int abbreviate_commands = 0, rebase_cousins = -1, ret = 0;
const char *onto = NULL, *onto_name = NULL, *restrict_revision = NULL,
*squash_onto = NULL, *upstream = NULL, *head_name = NULL,
*switch_to = NULL, *cmd = NULL;
char *raw_strategies = NULL;
+   enum {
+   NONE = 0, CONTINUE, SKIP, EDIT_TODO, SHOW_CURRENT_PATCH
+   } command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
OPT_BOOL(0, "keep-empty", _empty, N_("keep empty 
commits")),
@@ -151,6 +154,13 @@ int cmd_rebase__interactive(int argc, const char **argv, 
const char *prefix)
 N_("move commits that begin with squash!/fixup!")),
OPT_BOOL(0, "signoff", , N_("sign commits")),
OPT__VERBOSE(, N_("be verbose")),
+   OPT_CMDMODE(0, "continue", , N_("continue rebase"),
+   CONTINUE),
+   OPT_CMDMODE(0, "skip", , N_("skip commit"), SKIP),
+   OPT_CMDMODE(0, "edit-todo", , N_("edit the todo list"),
+   EDIT_TODO),
+   OPT_CMDMODE(0, "show-current-patch", , N_("show the 
current patch"),
+   SHOW_CURRENT_PATCH),
OPT_STRING(0, "onto", , N_("onto"), N_("onto")),
OPT_STRING(0, "restrict-revision", _revision,
   N_("restrict-revision"), N_("restrict revision")),
@@ -198,10 +208,39 @@ int cmd_rebase__interactive(int argc, const char **argv, 
const char *prefix)
warning(_("--[no-]rebase-cousins has no effect without "
  "--rebase-merges"));
 
-   if (!onto && !upstream)
-   die(_("a base commit must be provided with --upstream or 
--onto"));
+   switch (command) {
+   case NONE:
+   if (!onto && !upstream)
+   die(_("a base commit must be provided with --upstream 
or --onto"));
+
+   ret = do_interactive_rebase(, flags, switch_to, upstream, 
onto,
+   onto_name, squash_onto, head_name, 
restrict_revision,
+   raw_strategies, cmd, autosquash);
+   break;
+   case SKIP: {
+   struct string_list merge_rr = STRING_LIST_INIT_DUP;
+
+   rerere_clear(_rr);
+   /* fallthrough */
+   case CONTINUE:
+   ret = sequencer_continue();
+   break;
+   }
+   case EDIT_TODO:
+   ret = edit_todo_list(flags);
+   break;
+   case SHOW_CURRENT_PATCH: {
+   struct child_process cmd = CHILD_PROCESS_INIT;
+
+   cmd.git_cmd = 1;
+   argv_array_pushl(, "show", "REBASE_HEAD", "--", NULL);
+   ret = run_command();
+
+   break;
+   }
+   default:
+   BUG("invalid command '%d'", command);
+   }
 
-   return !!do_interactive_rebase(, flags, switch_to, upstream, onto,
-  onto_name, squash_onto, head_name, 
restrict_revision,
-  raw_strategies, cmd, autosquash);
+   return !!ret;
 }
diff --git a/git-rebase.sh b/git-rebase.sh
index 51a6db7daa..6e1e413cf2 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -200,19 +200,56 @@ finish_rebase () {
rm -rf "$state_dir"
 }
 
+run_interactive () {
+   GIT_CHERRY_PICK_HELP="$resolvemsg"
+   export GIT_CHERRY_PICK_HELP
+
+   test -n "$keep_empty" && keep_empty="--keep-empty"
+   test -n "$rebase_merges" && rebase_merges="--rebase-merges"
+   test -n "$rebase_cousins" && rebase_cousins="--rebase-cousins"
+   test -n "$autosquash" && autosquash="--autosquash"
+   test -n "$verbose" && verbose="--verbose"
+   test -n "$force_rebase" && force_rebase="--no-ff"
+   test -n "$restrict_revision" && \
+   

[GSoC][PATCH v8 11/20] rebase -i: rewrite complete_action() in C

[Alban Gruin]

This rewrites complete_action() from shell to C.

A new mode is added to rebase--helper (`--complete-action`), as well as
a new flag (`--autosquash`).

Finally, complete_action() is stripped from git-rebase--interactive.sh.

The original complete_action() would return the code 2 when the todo
list contained no actions.  This was a special case for rebase -i and
-p; git-rebase.sh would then apply the autostash, delete the state
directory, and die with the message "Nothing to do".  This cleanup is
rewritten in C instead of returning 2.  As rebase -i no longer returns
2, the comment describing this behaviour in git-rebase.sh is updated to
reflect this change.

The message "Nothing to do" is now printed with error(), and so becomes
"error: nothing to do".  Some tests in t3404 check this value, so they
are updated to fit this change.

The first check might seem useless as we write "noop" to the todo list
if it is empty.  Actually, the todo list might contain commented
commands (ie. empty commits).  In this case, complete_action() won’t
write "noop", and will abort without starting the editor.

Signed-off-by: Alban Gruin 
---
The changes are due to conflict resolution, no real changes otherwise.

 builtin/rebase--helper.c  |  12 +++-
 git-rebase--interactive.sh|  53 ++---
 git-rebase.sh |   2 +-
 sequencer.c   | 104 ++
 sequencer.h   |   4 ++
 t/t3404-rebase-interactive.sh |   2 +-
 6 files changed, 124 insertions(+), 53 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index bed3dd2b95..01b958 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,13 +13,13 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+   unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
-   CHECKOUT_ONTO
+   CHECKOUT_ONTO, COMPLETE_ACTION
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -29,6 +29,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge 
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
 N_("keep original branch points of cousins")),
+   OPT_BOOL(0, "autosquash", ,
+N_("move commits that begin with squash!/fixup!")),
OPT__VERBOSE(, N_("be verbose")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
@@ -57,6 +59,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("prepare the branch to be rebased"), 
PREPARE_BRANCH),
OPT_CMDMODE(0, "checkout-onto", ,
N_("checkout a commit"), CHECKOUT_ONTO),
+   OPT_CMDMODE(0, "complete-action", ,
+   N_("complete the action"), COMPLETE_ACTION),
OPT_END()
};
 
@@ -110,5 +114,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!prepare_branch_to_be_rebased(, argv[1]);
if (command == CHECKOUT_ONTO && argc == 4)
return !!checkout_onto(, argv[1], argv[2], argv[3]);
+   if (command == COMPLETE_ACTION && argc == 6)
+   return !!complete_action(, flags, argv[1], argv[2], 
argv[3],
+argv[4], argv[5], autosquash);
+
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index b68f108f28..59dc4888a6 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -127,54 +127,6 @@ init_revisions_and_shortrevisions () {
fi
 }
 
-complete_action() {
-   test -s "$todo" || echo noop >> "$todo"
-   test -z "$autosquash" || git rebase--helper --rearrange-squash || exit
-   test -n "$cmd" && git rebase--helper --add-exec-commands "$cmd"
-
-   todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
-   todocount=${todocount##* }
-
-cat >>"$todo" <"$todo" ||
die "$(gettext "Could not generate todo list")"
 
-   complete_action
+   exec git rebase--helper --complete-action "$shortrevisions" 
"$onto_name" \
+   "$shortonto" "$orig_head" "$cmd" 

[GSoC][PATCH v8 09/20] sequencer: change the way skip_unnecessary_picks() returns its result

[Alban Gruin]

Instead of skip_unnecessary_picks() printing its result to stdout, it
returns it into a struct object_id, as the rewrite of complete_action()
(to come in the next commit) will need it.

rebase--helper then is modified to fit this change.

Signed-off-by: Alban Gruin 
---
The changes are due to conflict resolution, no real changes otherwise.

 builtin/rebase--helper.c | 10 --
 sequencer.c  | 13 ++---
 sequencer.h  |  2 +-
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 313092c465..bed3dd2b95 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -90,8 +90,14 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!transform_todos(flags);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
-   if (command == SKIP_UNNECESSARY_PICKS && argc == 1)
-   return !!skip_unnecessary_picks();
+   if (command == SKIP_UNNECESSARY_PICKS && argc == 1) {
+   struct object_id oid;
+   int ret = skip_unnecessary_picks();
+
+   if (!ret)
+   puts(oid_to_hex());
+   return !!ret;
+   }
if (command == REARRANGE_SQUASH && argc == 1)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
diff --git a/sequencer.c b/sequencer.c
index c7f930ab9c..f2cda5593d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4593,17 +4593,17 @@ static int rewrite_file(const char *path, const char 
*buf, size_t len)
 }
 
 /* skip picking commits whose parents are unchanged */
-int skip_unnecessary_picks(void)
+int skip_unnecessary_picks(struct object_id *output_oid)
 {
const char *todo_file = rebase_path_todo();
struct strbuf buf = STRBUF_INIT;
struct todo_list todo_list = TODO_LIST_INIT;
-   struct object_id onto_oid, *oid = _oid, *parent_oid;
+   struct object_id *parent_oid;
int fd, i;
 
if (!read_oneliner(, rebase_path_onto(), 0))
return error(_("could not read 'onto'"));
-   if (get_oid(buf.buf, _oid)) {
+   if (get_oid(buf.buf, output_oid)) {
strbuf_release();
return error(_("need a HEAD to fixup"));
}
@@ -4633,9 +4633,9 @@ int skip_unnecessary_picks(void)
if (item->commit->parents->next)
break; /* merge commit */
parent_oid = >commit->parents->item->object.oid;
-   if (!oideq(parent_oid, oid))
+   if (!oideq(parent_oid, output_oid))
break;
-   oid = >commit->object.oid;
+   oidcpy(output_oid, >commit->object.oid);
}
if (i > 0) {
int offset = get_item_line_offset(_list, i);
@@ -4664,11 +4664,10 @@ int skip_unnecessary_picks(void)
 
todo_list.current = i;
if (is_fixup(peek_command(_list, 0)))
-   record_in_rewritten(oid, peek_command(_list, 0));
+   record_in_rewritten(output_oid, 
peek_command(_list, 0));
}
 
todo_list_release(_list);
-   printf("%s\n", oid_to_hex(oid));
 
return 0;
 }
diff --git a/sequencer.h b/sequencer.h
index 5fa89edd3b..6b0885db57 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -96,7 +96,7 @@ int sequencer_add_exec_commands(const char *command);
 int transform_todos(unsigned flags);
 enum missing_commit_check_level get_missing_commit_check_level(void);
 int check_todo_list(void);
-int skip_unnecessary_picks(void);
+int skip_unnecessary_picks(struct object_id *output_oid);
 int rearrange_squash(void);
 
 extern const char sign_off_header[];
-- 
2.19.0

[GSoC][PATCH v8 13/20] rebase -i: implement the logic to initialize $revisions in C

[Alban Gruin]

This rewrites the part of init_revisions_and_shortrevisions() needed by
`--make-script` from shell to C.  The new version is called
get_revision_ranges(), and is a static function inside of
rebase--helper.c.  As this does not initialize $shortrevision, the
original shell version is not yet stripped.

Unlike init_revisions_and_shortrevisions(), get_revision_ranges()
doesn’t write $squash_onto to the state directory, it’s done by the
handler of `--make-script` instead.

Finally, this drops the $revision argument passed to `--make-script` in
git-rebase--interactive.sh, and rebase--helper is changed accordingly.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--helper.c   | 56 --
 git-rebase--interactive.sh |  4 ++-
 2 files changed, 56 insertions(+), 4 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index e1460136f5..acc71a6f99 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -4,6 +4,25 @@
 #include "parse-options.h"
 #include "sequencer.h"
 #include "rebase-interactive.h"
+#include "argv-array.h"
+
+static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
+
+static int get_revision_ranges(const char *upstream, const char *onto,
+  const char **head_hash,
+  char **revisions)
+{
+   const char *base_rev = upstream ? upstream : onto;
+   struct object_id orig_head;
+
+   if (get_oid("HEAD", _head))
+   return error(_("no HEAD?"));
+
+   *head_hash = find_unique_abbrev(_head, GIT_MAX_HEXSZ);
+   *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+   return 0;
+}
 
 static const char * const builtin_rebase_helper_usage[] = {
N_("git rebase--helper []"),
@@ -14,7 +33,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
-   int abbreviate_commands = 0, rebase_cousins = -1;
+   int abbreviate_commands = 0, rebase_cousins = -1, ret;
+   const char *head_hash = NULL, *onto = NULL, *restrict_revision = NULL,
+   *squash_onto = NULL, *upstream = NULL;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC, EDIT_TODO, 
PREPARE_BRANCH,
@@ -54,6 +75,13 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("prepare the branch to be rebased"), 
PREPARE_BRANCH),
OPT_CMDMODE(0, "complete-action", ,
N_("complete the action"), COMPLETE_ACTION),
+   OPT_STRING(0, "onto", , N_("onto"), N_("onto")),
+   OPT_STRING(0, "restrict-revision", _revision,
+  N_("restrict-revision"), N_("restrict revision")),
+   OPT_STRING(0, "squash-onto", _onto, N_("squash-onto"),
+  N_("squash onto")),
+   OPT_STRING(0, "upstream", , N_("upstream"),
+  N_("the upstream commit")),
OPT_END()
};
 
@@ -81,8 +109,30 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!sequencer_continue();
if (command == ABORT && argc == 1)
return !!sequencer_remove_state();
-   if (command == MAKE_SCRIPT && argc > 1)
-   return !!sequencer_make_script(stdout, argc, argv, flags);
+   if (command == MAKE_SCRIPT && argc == 1) {
+   char *revisions = NULL;
+   struct argv_array make_script_args = ARGV_ARRAY_INIT;
+
+   if (!upstream && squash_onto)
+   write_file(path_squash_onto(), "%s\n", squash_onto);
+
+   ret = get_revision_ranges(upstream, onto, _hash, 
);
+   if (ret)
+   return ret;
+
+   argv_array_pushl(_script_args, "", revisions, NULL);
+   if (restrict_revision)
+   argv_array_push(_script_args, restrict_revision);
+
+   ret = sequencer_make_script(stdout,
+   make_script_args.argc, 
make_script_args.argv,
+   flags);
+
+   free(revisions);
+   argv_array_clear(_script_args);
+
+   return !!ret;
+   }
if ((command == SHORTEN_OIDS || command == EXPAND_OIDS) && argc == 1)
return !!transform_todos(flags);
if (command == CHECK_TODO_LIST && argc == 1)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 0d66c0f8b8..4ca47aed1e 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -92,7 +92,9 @@ git_rebase__interactive () {
git rebase--helper --make-script ${keep_empty:+--keep-empty} \
  

[GSoC][PATCH v8 14/20] rebase -i: rewrite the rest of init_revisions_and_shortrevisions() in C

[Alban Gruin]

This rewrites the part of init_revisions_and_shortrevisions() needed by
`--complete-action` (which initialize $shortrevisions) from shell to C.

When `upstream` is empty, it means that the user launched a `rebase
--root`, and `onto` contains the ID of an empty commit.  As a range
between an empty commit and `head` is not really meaningful, `onto` is
not used to initialize `shortrevisions` in this case.

The corresponding arguments passed to `--complete-action` are then
dropped, and init_revisions_and_shortrevisions() is stripped from
git-rebase--interactive.sh

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--helper.c   | 40 --
 git-rebase--interactive.sh | 27 -
 2 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index acc71a6f99..0716bbfd78 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -10,7 +10,7 @@ static GIT_PATH_FUNC(path_squash_onto, 
"rebase-merge/squash-onto")
 
 static int get_revision_ranges(const char *upstream, const char *onto,
   const char **head_hash,
-  char **revisions)
+  char **revisions, char **shortrevisions)
 {
const char *base_rev = upstream ? upstream : onto;
struct object_id orig_head;
@@ -19,7 +19,25 @@ static int get_revision_ranges(const char *upstream, const 
char *onto,
return error(_("no HEAD?"));
 
*head_hash = find_unique_abbrev(_head, GIT_MAX_HEXSZ);
-   *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+   if (revisions)
+   *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+   if (shortrevisions) {
+   const char *shorthead;
+
+   shorthead = find_unique_abbrev(_head, DEFAULT_ABBREV);
+
+   if (upstream) {
+   const char *shortrev;
+   struct object_id rev_oid;
+
+   get_oid(base_rev, _oid);
+   shortrev = find_unique_abbrev(_oid, DEFAULT_ABBREV);
+
+   *shortrevisions = xstrfmt("%s..%s", shortrev, 
shorthead);
+   } else
+   *shortrevisions = xstrdup(shorthead);
+   }
 
return 0;
 }
@@ -116,7 +134,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (!upstream && squash_onto)
write_file(path_squash_onto(), "%s\n", squash_onto);
 
-   ret = get_revision_ranges(upstream, onto, _hash, 
);
+   ret = get_revision_ranges(upstream, onto, _hash, 
, NULL);
if (ret)
return ret;
 
@@ -145,9 +163,19 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!edit_todo_list(flags);
if (command == PREPARE_BRANCH && argc == 2)
return !!prepare_branch_to_be_rebased(, argv[1]);
-   if (command == COMPLETE_ACTION && argc == 6)
-   return !!complete_action(, flags, argv[1], argv[2], 
argv[3],
-argv[4], argv[5], autosquash);
+   if (command == COMPLETE_ACTION && argc == 3) {
+   char *shortrevisions = NULL;
+
+   ret = get_revision_ranges(upstream, onto, _hash, NULL, 
);
+   if (ret)
+   return ret;
+
+   ret = complete_action(, flags, shortrevisions, argv[1], 
onto,
+ head_hash, argv[2], autosquash);
+
+   free(shortrevisions);
+   return !!ret;
+   }
 
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 4ca47aed1e..08e9a21c2f 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -60,23 +60,6 @@ init_basic_state () {
write_basic_state
 }
 
-init_revisions_and_shortrevisions () {
-   shorthead=$(git rev-parse --short $orig_head)
-   shortonto=$(git rev-parse --short $onto)
-   if test -z "$rebase_root"
-   # this is now equivalent to ! -z "$upstream"
-   then
-   shortupstream=$(git rev-parse --short $upstream)
-   revisions=$upstream...$orig_head
-   shortrevisions=$shortupstream..$shorthead
-   else
-   revisions=$onto...$orig_head
-   shortrevisions=$shorthead
-   test -z "$squash_onto" ||
-   echo "$squash_onto" >"$state_dir"/squash-onto
-   fi
-}
-
 git_rebase__interactive () {
initiate_action "$action"
ret=$?
@@ -87,8 +70,6 @@ git_rebase__interactive () {
git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
init_basic_state
 
-   init_revisions_and_shortrevisions
-
git rebase--helper --make-script 

[GSoC][PATCH v8 15/20] rebase -i: rewrite write_basic_state() in C

[Alban Gruin]

This rewrites write_basic_state() from git-rebase.sh in C.  This is the
first step in the conversion of init_basic_state(), hence the mode in
rebase--helper.c is called INIT_BASIC_STATE.  init_basic_state() will be
converted in the next commit.

The part of read_strategy_opts() that parses the stategy options is
moved to a new function to allow its use in rebase--helper.c.

Finally, the call to write_basic_state() is removed from
git-rebase--interactive.sh, replaced by a call to `--init-basic-state`.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--helper.c   | 28 +-
 git-rebase--interactive.sh |  7 +++-
 sequencer.c| 77 --
 sequencer.h|  4 ++
 4 files changed, 102 insertions(+), 14 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 0716bbfd78..63c5086e42 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -5,6 +5,8 @@
 #include "sequencer.h"
 #include "rebase-interactive.h"
 #include "argv-array.h"
+#include "rerere.h"
+#include "alias.h"
 
 static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
 
@@ -53,11 +55,12 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
int abbreviate_commands = 0, rebase_cousins = -1, ret;
const char *head_hash = NULL, *onto = NULL, *restrict_revision = NULL,
-   *squash_onto = NULL, *upstream = NULL;
+   *squash_onto = NULL, *upstream = NULL, *head_name = NULL;
+   char *raw_strategies = NULL;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC, EDIT_TODO, 
PREPARE_BRANCH,
-   COMPLETE_ACTION
+   COMPLETE_ACTION, INIT_BASIC_STATE
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -69,6 +72,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
 N_("keep original branch points of cousins")),
OPT_BOOL(0, "autosquash", ,
 N_("move commits that begin with squash!/fixup!")),
+   OPT_BOOL(0, "signoff", , N_("sign commits")),
OPT__VERBOSE(, N_("be verbose")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
@@ -93,6 +97,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("prepare the branch to be rebased"), 
PREPARE_BRANCH),
OPT_CMDMODE(0, "complete-action", ,
N_("complete the action"), COMPLETE_ACTION),
+   OPT_CMDMODE(0, "init-basic-state", ,
+   N_("initialise the rebase state"), 
INIT_BASIC_STATE),
OPT_STRING(0, "onto", , N_("onto"), N_("onto")),
OPT_STRING(0, "restrict-revision", _revision,
   N_("restrict-revision"), N_("restrict revision")),
@@ -100,6 +106,14 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
   N_("squash onto")),
OPT_STRING(0, "upstream", , N_("upstream"),
   N_("the upstream commit")),
+   OPT_STRING(0, "head-name", _name, N_("head-name"), 
N_("head name")),
+   OPT_STRING('S', "gpg-sign", _sign, N_("gpg-sign"),
+  N_("GPG-sign commits")),
+   OPT_STRING(0, "strategy", , N_("strategy"),
+  N_("rebase strategy")),
+   OPT_STRING(0, "strategy-opts", _strategies, 
N_("strategy-opts"),
+  N_("strategy options")),
+   OPT_RERERE_AUTOUPDATE(_rerere_auto),
OPT_END()
};
 
@@ -176,6 +190,16 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
free(shortrevisions);
return !!ret;
}
+   if (command == INIT_BASIC_STATE) {
+   if (raw_strategies)
+   parse_strategy_opts(, raw_strategies);
+
+   ret = get_revision_ranges(upstream, onto, _hash, NULL, 
NULL);
+   if (ret)
+   return ret;
+
+   return !!write_basic_state(, head_name, onto, head_hash);
+   }
 
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 08e9a21c2f..6367da66e2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -57,7 +57,6 @@ init_basic_state () {
rm -f "$(git rev-parse --git-path REBASE_HEAD)"
 
: > "$state_dir"/interactive || die "$(gettext "Could not mark as 
interactive")"
-   write_basic_state
 

[GSoC][PATCH v8 17/20] rebase -i: implement the main part of interactive rebase as a builtin

[Alban Gruin]

This rewrites the part of interactive rebase which initializes the
basic state, make the script and complete the action, as a buitin, named
git-rebase--interactive2 for now.  Others modes (`--continue`,
`--edit-todo`, etc.) will be rewritten in the next commit.

git-rebase--interactive.sh is modified to call git-rebase--interactive2
instead of git-rebase--helper.

Signed-off-by: Alban Gruin 
---
 .gitignore |   1 +
 Makefile   |   1 +
 builtin.h  |   1 +
 builtin/rebase--interactive2.c | 207 +
 git-rebase--interactive.sh |  41 ---
 git.c  |   1 +
 6 files changed, 233 insertions(+), 19 deletions(-)
 create mode 100644 builtin/rebase--interactive2.c

diff --git a/.gitignore b/.gitignore
index 9d1363a1eb..e33c09d52a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -120,6 +120,7 @@
 /git-rebase--am
 /git-rebase--helper
 /git-rebase--interactive
+/git-rebase--interactive2
 /git-rebase--merge
 /git-rebase--preserve-merges
 /git-receive-pack
diff --git a/Makefile b/Makefile
index ff7f01634f..bd10c87075 100644
--- a/Makefile
+++ b/Makefile
@@ -1082,6 +1082,7 @@ BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/range-diff.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase--interactive2.o
 BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index 962f0489ab..b79265d3d4 100644
--- a/builtin.h
+++ b/builtin.h
@@ -204,6 +204,7 @@ extern int cmd_pull(int argc, const char **argv, const char 
*prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_range_diff(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase__interactive(int argc, const char **argv, const char 
*prefix);
 extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase--interactive2.c b/builtin/rebase--interactive2.c
new file mode 100644
index 00..45336073a8
--- /dev/null
+++ b/builtin/rebase--interactive2.c
@@ -0,0 +1,207 @@
+#include "builtin.h"
+#include "cache.h"
+#include "config.h"
+#include "parse-options.h"
+#include "sequencer.h"
+#include "rebase-interactive.h"
+#include "argv-array.h"
+#include "refs.h"
+#include "rerere.h"
+#include "run-command.h"
+
+static GIT_PATH_FUNC(path_state_dir, "rebase-merge/")
+static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
+static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
+
+static int get_revision_ranges(const char *upstream, const char *onto,
+  const char **head_hash,
+  char **revisions, char **shortrevisions)
+{
+   const char *base_rev = upstream ? upstream : onto, *shorthead;
+   struct object_id orig_head;
+
+   if (get_oid("HEAD", _head))
+   return error(_("no HEAD?"));
+
+   *head_hash = find_unique_abbrev(_head, GIT_MAX_HEXSZ);
+   *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
+
+   shorthead = find_unique_abbrev(_head, DEFAULT_ABBREV);
+
+   if (upstream) {
+   const char *shortrev;
+   struct object_id rev_oid;
+
+   get_oid(base_rev, _oid);
+   shortrev = find_unique_abbrev(_oid, DEFAULT_ABBREV);
+
+   *shortrevisions = xstrfmt("%s..%s", shortrev, shorthead);
+   } else
+   *shortrevisions = xstrdup(shorthead);
+
+   return 0;
+}
+
+static int init_basic_state(struct replay_opts *opts, const char *head_name,
+   const char *onto, const char *orig_head)
+{
+   FILE *interactive;
+
+   if (!is_directory(path_state_dir()) && 
mkdir_in_gitdir(path_state_dir()))
+   return error_errno(_("could not create temporary %s"), 
path_state_dir());
+
+   delete_reflog("REBASE_HEAD");
+
+   interactive = fopen(path_interactive(), "w");
+   if (!interactive)
+   return error_errno(_("could not mark as interactive"));
+   fclose(interactive);
+
+   return write_basic_state(opts, head_name, onto, orig_head);
+}
+
+static int do_interactive_rebase(struct replay_opts *opts, unsigned flags,
+const char *switch_to, const char *upstream,
+const char *onto, const char *onto_name,
+const char *squash_onto, const char *head_name,
+const char *restrict_revision, char 
*raw_strategies,
+const char *cmd, unsigned autosquash)
+{
+   int ret;
+   const char 

[GSoC][PATCH v8 16/20] rebase -i: rewrite init_basic_state() in C

[Alban Gruin]

This rewrites init_basic_state() from shell to C.  The call to
write_basic_state() in cmd_rebase__helper() is replaced by a call to the
new function.

The shell version is then stripped from git-rebase--interactive.sh.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--helper.c   | 23 ++-
 git-rebase--interactive.sh |  9 -
 2 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 63c5086e42..f8128037d3 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -5,10 +5,13 @@
 #include "sequencer.h"
 #include "rebase-interactive.h"
 #include "argv-array.h"
+#include "refs.h"
 #include "rerere.h"
 #include "alias.h"
 
+static GIT_PATH_FUNC(path_state_dir, "rebase-merge/")
 static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
+static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
 
 static int get_revision_ranges(const char *upstream, const char *onto,
   const char **head_hash,
@@ -44,6 +47,24 @@ static int get_revision_ranges(const char *upstream, const 
char *onto,
return 0;
 }
 
+static int init_basic_state(struct replay_opts *opts, const char *head_name,
+   const char *onto, const char *orig_head)
+{
+   FILE *interactive;
+
+   if (!is_directory(path_state_dir()) && 
mkdir_in_gitdir(path_state_dir()))
+   return error_errno(_("could not create temporary %s"), 
path_state_dir());
+
+   delete_reflog("REBASE_HEAD");
+
+   interactive = fopen(path_interactive(), "w");
+   if (!interactive)
+   return error_errno(_("could not mark as interactive"));
+   fclose(interactive);
+
+   return write_basic_state(opts, head_name, onto, orig_head);
+}
+
 static const char * const builtin_rebase_helper_usage[] = {
N_("git rebase--helper []"),
NULL
@@ -198,7 +219,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (ret)
return ret;
 
-   return !!write_basic_state(, head_name, onto, head_hash);
+   return !!init_basic_state(, head_name, onto, head_hash);
}
 
usage_with_options(builtin_rebase_helper_usage, options);
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 6367da66e2..761be95ed1 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -51,14 +51,6 @@ initiate_action () {
esac
 }
 
-init_basic_state () {
-   orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
-   mkdir -p "$state_dir" || die "$(eval_gettext "Could not create 
temporary \$state_dir")"
-   rm -f "$(git rev-parse --git-path REBASE_HEAD)"
-
-   : > "$state_dir"/interactive || die "$(gettext "Could not mark as 
interactive")"
-}
-
 git_rebase__interactive () {
initiate_action "$action"
ret=$?
@@ -67,7 +59,6 @@ git_rebase__interactive () {
fi
 
git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
-   init_basic_state
 
git rebase--helper --init-basic-state ${upstream:+--upstream 
"$upstream"} \
${onto:+--onto "$onto"} ${head_name:+--head-name "$head_name"} \
-- 
2.19.0

[GSoC][PATCH v8 20/20] rebase -i: move rebase--helper modes to rebase--interactive

[Alban Gruin]

This moves the rebase--helper modes still used by
git-rebase--preserve-merges.sh (`--shorten-ids`, `--expand-ids`,
`--check-todo-list`, `--rearrange-squash` and `--add-exec-commands`) to
rebase--interactive.c.

git-rebase--preserve-merges.sh is modified accordingly, and
rebase--helper.c is removed as it is useless now.

Signed-off-by: Alban Gruin 
---
The changes are due to the rebase, no real changes otherwise.

 .gitignore |   1 -
 Makefile   |   1 -
 builtin.h  |   1 -
 builtin/rebase--helper.c   | 226 -
 builtin/rebase--interactive.c  |  27 +++-
 git-rebase--preserve-merges.sh |  10 +-
 git.c  |   1 -
 7 files changed, 31 insertions(+), 236 deletions(-)
 delete mode 100644 builtin/rebase--helper.c

diff --git a/.gitignore b/.gitignore
index 9d1363a1eb..1c0394f576 100644
--- a/.gitignore
+++ b/.gitignore
@@ -118,7 +118,6 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
-/git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
 /git-rebase--preserve-merges
diff --git a/Makefile b/Makefile
index dc26276747..ec4ee78b3f 100644
--- a/Makefile
+++ b/Makefile
@@ -1081,7 +1081,6 @@ BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/range-diff.o
 BUILTIN_OBJS += builtin/read-tree.o
-BUILTIN_OBJS += builtin/rebase--helper.o
 BUILTIN_OBJS += builtin/rebase--interactive.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
diff --git a/builtin.h b/builtin.h
index b79265d3d4..f6a7d82d73 100644
--- a/builtin.h
+++ b/builtin.h
@@ -205,7 +205,6 @@ extern int cmd_push(int argc, const char **argv, const char 
*prefix);
 extern int cmd_range_diff(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_rebase__interactive(int argc, const char **argv, const char 
*prefix);
-extern int cmd_rebase__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
deleted file mode 100644
index f8128037d3..00
--- a/builtin/rebase--helper.c
+++ /dev/null
@@ -1,226 +0,0 @@
-#include "builtin.h"
-#include "cache.h"
-#include "config.h"
-#include "parse-options.h"
-#include "sequencer.h"
-#include "rebase-interactive.h"
-#include "argv-array.h"
-#include "refs.h"
-#include "rerere.h"
-#include "alias.h"
-
-static GIT_PATH_FUNC(path_state_dir, "rebase-merge/")
-static GIT_PATH_FUNC(path_squash_onto, "rebase-merge/squash-onto")
-static GIT_PATH_FUNC(path_interactive, "rebase-merge/interactive")
-
-static int get_revision_ranges(const char *upstream, const char *onto,
-  const char **head_hash,
-  char **revisions, char **shortrevisions)
-{
-   const char *base_rev = upstream ? upstream : onto;
-   struct object_id orig_head;
-
-   if (get_oid("HEAD", _head))
-   return error(_("no HEAD?"));
-
-   *head_hash = find_unique_abbrev(_head, GIT_MAX_HEXSZ);
-
-   if (revisions)
-   *revisions = xstrfmt("%s...%s", base_rev, *head_hash);
-   if (shortrevisions) {
-   const char *shorthead;
-
-   shorthead = find_unique_abbrev(_head, DEFAULT_ABBREV);
-
-   if (upstream) {
-   const char *shortrev;
-   struct object_id rev_oid;
-
-   get_oid(base_rev, _oid);
-   shortrev = find_unique_abbrev(_oid, DEFAULT_ABBREV);
-
-   *shortrevisions = xstrfmt("%s..%s", shortrev, 
shorthead);
-   } else
-   *shortrevisions = xstrdup(shorthead);
-   }
-
-   return 0;
-}
-
-static int init_basic_state(struct replay_opts *opts, const char *head_name,
-   const char *onto, const char *orig_head)
-{
-   FILE *interactive;
-
-   if (!is_directory(path_state_dir()) && 
mkdir_in_gitdir(path_state_dir()))
-   return error_errno(_("could not create temporary %s"), 
path_state_dir());
-
-   delete_reflog("REBASE_HEAD");
-
-   interactive = fopen(path_interactive(), "w");
-   if (!interactive)
-   return error_errno(_("could not mark as interactive"));
-   fclose(interactive);
-
-   return write_basic_state(opts, head_name, onto, orig_head);
-}
-
-static const char * const builtin_rebase_helper_usage[] = {
-   N_("git rebase--helper []"),
-   NULL
-};
-
-int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
-{
-   struct replay_opts opts = REPLAY_OPTS_INIT;
-   unsigned flags = 0, keep_empty = 0, rebase_merges = 0, autosquash = 0;
-   

[GSoC][PATCH v8 19/20] rebase -i: remove git-rebase--interactive.sh

[Alban Gruin]

This removes git-rebase--interactive.sh, as its functionnality has been
replaced by git-rebase--interactive2.

git-rebase--interactive2.c is then renamed to git-rebase--interactive.c.

Signed-off-by: Alban Gruin 
---
The changes are due to the rebase, no real changes otherwise.

 .gitignore|  1 -
 Makefile  |  4 +-
 ...--interactive2.c => rebase--interactive.c} |  0
 git-rebase--interactive.sh| 84 ---
 git-rebase.sh |  2 +-
 git.c |  2 +-
 6 files changed, 3 insertions(+), 90 deletions(-)
 rename builtin/{rebase--interactive2.c => rebase--interactive.c} (100%)
 delete mode 100644 git-rebase--interactive.sh

diff --git a/.gitignore b/.gitignore
index e33c09d52a..9d1363a1eb 100644
--- a/.gitignore
+++ b/.gitignore
@@ -120,7 +120,6 @@
 /git-rebase--am
 /git-rebase--helper
 /git-rebase--interactive
-/git-rebase--interactive2
 /git-rebase--merge
 /git-rebase--preserve-merges
 /git-receive-pack
diff --git a/Makefile b/Makefile
index bd10c87075..dc26276747 100644
--- a/Makefile
+++ b/Makefile
@@ -624,7 +624,6 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
-SCRIPT_LIB += git-rebase--interactive
 SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
@@ -1082,8 +1081,8 @@ BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/range-diff.o
 BUILTIN_OBJS += builtin/read-tree.o
-BUILTIN_OBJS += builtin/rebase--interactive2.o
 BUILTIN_OBJS += builtin/rebase--helper.o
+BUILTIN_OBJS += builtin/rebase--interactive.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
@@ -2422,7 +2421,6 @@ XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH)
 LOCALIZED_SH += git-parse-remote.sh
-LOCALIZED_SH += git-rebase--interactive.sh
 LOCALIZED_SH += git-rebase--preserve-merges.sh
 LOCALIZED_SH += git-sh-setup.sh
 LOCALIZED_PERL = $(SCRIPT_PERL)
diff --git a/builtin/rebase--interactive2.c b/builtin/rebase--interactive.c
similarity index 100%
rename from builtin/rebase--interactive2.c
rename to builtin/rebase--interactive.c
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
deleted file mode 100644
index e87d708a4d..00
--- a/git-rebase--interactive.sh
+++ /dev/null
@@ -1,84 +0,0 @@
-# This shell script fragment is sourced by git-rebase to implement
-# its interactive mode.  "git rebase --interactive" makes it easy
-# to fix up commits in the middle of a series and rearrange commits.
-#
-# Copyright (c) 2006 Johannes E. Schindelin
-#
-# The original idea comes from Eric W. Biederman, in
-# https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
-#
-# The file containing rebase commands, comments, and empty lines.
-# This file is created by "git rebase -i" then edited by the user.  As
-# the lines are processed, they are removed from the front of this
-# file and written to the tail of $done.
-todo="$state_dir"/git-rebase-todo
-
-GIT_CHERRY_PICK_HELP="$resolvemsg"
-export GIT_CHERRY_PICK_HELP
-
-# Initiate an action. If the cannot be any
-# further action it  may exec a command
-# or exit and not return.
-#
-# TODO: Consider a cleaner return model so it
-# never exits and always return 0 if process
-# is complete.
-#
-# Parameter 1 is the action to initiate.
-#
-# Returns 0 if the action was able to complete
-# and if 1 if further processing is required.
-initiate_action () {
-   case "$1" in
-   continue)
-   exec git rebase--helper ${force_rebase:+--no-ff} 
$allow_empty_message \
---continue
-   ;;
-   skip)
-   git rerere clear
-   exec git rebase--helper ${force_rebase:+--no-ff} 
$allow_empty_message \
---continue
-   ;;
-   edit-todo)
-   exec git rebase--helper --edit-todo
-   ;;
-   show-current-patch)
-   exec git show REBASE_HEAD --
-   ;;
-   *)
-   return 1 # continue
-   ;;
-   esac
-}
-
-git_rebase__interactive () {
-   initiate_action "$action"
-   ret=$?
-   if test $ret = 0; then
-   return 0
-   fi
-
-   test -n "$keep_empty" && keep_empty="--keep-empty"
-   test -n "$rebase_merges" && rebase_merges="--rebase-merges"
-   test -n "$rebase_cousins" && rebase_cousins="--rebase-cousins"
-   test -n "$autosquash" && autosquash="--autosquash"
-   test -n "$verbose" && verbose="--verbose"
-   test -n "$force_rebase" && force_rebase="--no-ff"
-   test -n "$restrict_revisions" && 
restrict_revisions="--restrict-revisions=^$restrict_revisions"
-   test -n 

[GSoC][PATCH v8 12/20] rebase -i: remove unused modes and functions

[Alban Gruin]

This removes the modes `--skip-unnecessary-picks`, `--append-todo-help`,
and `--checkout-onto` from rebase--helper.c, the functions of
git-rebase--interactive.sh that were rendered useless by the rewrite of
complete_action(), and append_todo_help_to_file() from
rebase-interactive.c.

skip_unnecessary_picks() and checkout_onto() becomes static, as they are
only used inside of the sequencer.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--helper.c   | 23 ++
 git-rebase--interactive.sh | 50 --
 rebase-interactive.c   | 22 -
 rebase-interactive.h   |  1 -
 sequencer.c|  8 +++---
 sequencer.h|  4 ---
 6 files changed, 6 insertions(+), 102 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 01b958..e1460136f5 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -17,9 +17,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
-   CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
-   CHECKOUT_ONTO, COMPLETE_ACTION
+   CHECK_TODO_LIST, REARRANGE_SQUASH, ADD_EXEC, EDIT_TODO, 
PREPARE_BRANCH,
+   COMPLETE_ACTION
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -44,21 +43,15 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("expand commit ids in the todo list"), EXPAND_OIDS),
OPT_CMDMODE(0, "check-todo-list", ,
N_("check the todo list"), CHECK_TODO_LIST),
-   OPT_CMDMODE(0, "skip-unnecessary-picks", ,
-   N_("skip unnecessary picks"), SKIP_UNNECESSARY_PICKS),
OPT_CMDMODE(0, "rearrange-squash", ,
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_CMDMODE(0, "add-exec-commands", ,
N_("insert exec commands in todo list"), ADD_EXEC),
-   OPT_CMDMODE(0, "append-todo-help", ,
-   N_("insert the help in the todo list"), 
APPEND_TODO_HELP),
OPT_CMDMODE(0, "edit-todo", ,
N_("edit the todo list during an interactive 
rebase"),
EDIT_TODO),
OPT_CMDMODE(0, "prepare-branch", ,
N_("prepare the branch to be rebased"), 
PREPARE_BRANCH),
-   OPT_CMDMODE(0, "checkout-onto", ,
-   N_("checkout a commit"), CHECKOUT_ONTO),
OPT_CMDMODE(0, "complete-action", ,
N_("complete the action"), COMPLETE_ACTION),
OPT_END()
@@ -94,26 +87,14 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!transform_todos(flags);
if (command == CHECK_TODO_LIST && argc == 1)
return !!check_todo_list();
-   if (command == SKIP_UNNECESSARY_PICKS && argc == 1) {
-   struct object_id oid;
-   int ret = skip_unnecessary_picks();
-
-   if (!ret)
-   puts(oid_to_hex());
-   return !!ret;
-   }
if (command == REARRANGE_SQUASH && argc == 1)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
-   if (command == APPEND_TODO_HELP && argc == 1)
-   return !!append_todo_help_to_file(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
if (command == PREPARE_BRANCH && argc == 2)
return !!prepare_branch_to_be_rebased(, argv[1]);
-   if (command == CHECKOUT_ONTO && argc == 4)
-   return !!checkout_onto(, argv[1], argv[2], argv[3]);
if (command == COMPLETE_ACTION && argc == 6)
return !!complete_action(, flags, argv[1], argv[2], 
argv[3],
 argv[4], argv[5], autosquash);
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 59dc4888a6..0d66c0f8b8 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -16,56 +16,6 @@ todo="$state_dir"/git-rebase-todo
 GIT_CHERRY_PICK_HELP="$resolvemsg"
 export GIT_CHERRY_PICK_HELP
 
-comment_char=$(git config --get core.commentchar 2>/dev/null)
-case "$comment_char" in
-'' | auto)
-   comment_char="#"
-   ;;
-?)
-   ;;
-*)
-   comment_char=$(echo "$comment_char" | cut -c1)
-   ;;
-esac
-
-die_abort () {
-   apply_autostash
-   rm -rf "$state_dir"

[GSoC][PATCH v8 07/20] rebase -i: rewrite checkout_onto() in C

[Alban Gruin]

This rewrites checkout_onto() from shell to C.

A new command (“checkout-onto”) is added to rebase--helper.c. The shell
version is then stripped.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--helper.c   |  7 ++-
 git-rebase--interactive.sh | 25 -
 sequencer.c| 19 +++
 sequencer.h|  3 +++
 4 files changed, 32 insertions(+), 22 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 0e76dadba6..7d9426d23c 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -18,7 +18,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
+   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH,
+   CHECKOUT_ONTO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -54,6 +55,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
EDIT_TODO),
OPT_CMDMODE(0, "prepare-branch", ,
N_("prepare the branch to be rebased"), 
PREPARE_BRANCH),
+   OPT_CMDMODE(0, "checkout-onto", ,
+   N_("checkout a commit"), CHECKOUT_ONTO),
OPT_END()
};
 
@@ -99,5 +102,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!edit_todo_list(flags);
if (command == PREPARE_BRANCH && argc == 2)
return !!prepare_branch_to_be_rebased(, argv[1]);
+   if (command == CHECKOUT_ONTO && argc == 4)
+   return !!checkout_onto(, argv[1], argv[2], argv[3]);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 77e972bb6c..b68f108f28 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -28,17 +28,6 @@ case "$comment_char" in
;;
 esac
 
-orig_reflog_action="$GIT_REFLOG_ACTION"
-
-comment_for_reflog () {
-   case "$orig_reflog_action" in
-   ''|rebase*)
-   GIT_REFLOG_ACTION="rebase -i ($1)"
-   export GIT_REFLOG_ACTION
-   ;;
-   esac
-}
-
 die_abort () {
apply_autostash
rm -rf "$state_dir"
@@ -70,14 +59,6 @@ collapse_todo_ids() {
git rebase--helper --shorten-ids
 }
 
-# Switch to the branch in $into and notify it in the reflog
-checkout_onto () {
-   comment_for_reflog start
-   GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
-   output git checkout $onto || die_abort "$(gettext "could not detach 
HEAD")"
-   git update-ref ORIG_HEAD $orig_head
-}
-
 get_missing_commit_check_level () {
check_level=$(git config --get rebase.missingCommitsCheck)
check_level=${check_level:-ignore}
@@ -176,7 +157,8 @@ EOF
 
git rebase--helper --check-todo-list || {
ret=$?
-   checkout_onto
+   git rebase--helper --checkout-onto "$onto_name" "$onto" \
+   "$orig_head" ${verbose:+--verbose}
exit $ret
}
 
@@ -186,7 +168,8 @@ EOF
onto="$(git rebase--helper --skip-unnecessary-picks)" ||
die "Could not skip unnecessary pick commands"
 
-   checkout_onto
+   git rebase--helper --checkout-onto "$onto_name" "$onto" "$orig_head" \
+   ${verbose:+--verbose}
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} $allow_empty_message \
 --continue
diff --git a/sequencer.c b/sequencer.c
index d8a59c50f8..c7f930ab9c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3315,6 +3315,25 @@ int prepare_branch_to_be_rebased(struct replay_opts 
*opts, const char *commit)
return 0;
 }
 
+int checkout_onto(struct replay_opts *opts,
+ const char *onto_name, const char *onto,
+ const char *orig_head)
+{
+   struct object_id oid;
+   const char *action = reflog_message(opts, "start", "checkout %s", 
onto_name);
+
+   if (get_oid(orig_head, ))
+   return error(_("%s: not a valid OID"), orig_head);
+
+   if (run_git_checkout(opts, onto, action)) {
+   apply_autostash(opts);
+   sequencer_remove_state(opts);
+   return error(_("could not detach HEAD"));
+   }
+
+   return update_ref(NULL, "ORIG_HEAD", , NULL, 0, 
UPDATE_REFS_MSG_ON_ERR);
+}
+
 static const char rescheduled_advice[] =
 N_("Could not execute the todo command\n"
 "\n"
diff --git a/sequencer.h b/sequencer.h
index 9f0ce21be7..5fa89edd3b 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -122,6 +122,9 @@ void 

[GSoC][PATCH v8 04/20] rebase -i: rewrite the edit-todo functionality in C

[Alban Gruin]

This rewrites the edit-todo functionality from shell to C.

To achieve that, a new command mode, `edit-todo`, is added, and the
`write-edit-todo` flag is removed, as the shell script does not need to
write the edit todo help message to the todo list anymore.

The shell version is then stripped in favour of a call to the helper.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--helper.c   | 13 -
 git-rebase--interactive.sh | 11 +--
 rebase-interactive.c   | 27 +++
 rebase-interactive.h   |  1 +
 4 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 05e73e71d4..731a64971d 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -13,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo 
= 0;
+   unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-   ADD_EXEC, APPEND_TODO_HELP
+   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -28,8 +28,6 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge 
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
 N_("keep original branch points of cousins")),
-   OPT_BOOL(0, "write-edit-todo", _edit_todo,
-N_("append the edit-todo message to the todo-list")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -50,6 +48,9 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("insert exec commands in todo list"), ADD_EXEC),
OPT_CMDMODE(0, "append-todo-help", ,
N_("insert the help in the todo list"), 
APPEND_TODO_HELP),
+   OPT_CMDMODE(0, "edit-todo", ,
+   N_("edit the todo list during an interactive 
rebase"),
+   EDIT_TODO),
OPT_END()
};
 
@@ -90,6 +91,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
if (command == APPEND_TODO_HELP && argc == 1)
-   return !!append_todo_help(write_edit_todo, keep_empty);
+   return !!append_todo_help(0, keep_empty);
+   if (command == EDIT_TODO && argc == 1)
+   return !!edit_todo_list(flags);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 94c23a7af2..2defe607f4 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -108,16 +108,7 @@ initiate_action () {
 --continue
;;
edit-todo)
-   git stripspace --strip-comments <"$todo" >"$todo".new
-   mv -f "$todo".new "$todo"
-   collapse_todo_ids
-   git rebase--helper --append-todo-help --write-edit-todo
-
-   git_sequence_editor "$todo" ||
-   die "$(gettext "Could not execute editor")"
-   expand_todo_ids
-
-   exit
+   exec git rebase--helper --edit-todo
;;
show-current-patch)
exec git show REBASE_HEAD --
diff --git a/rebase-interactive.c b/rebase-interactive.c
index d7996bc8d9..3f9468fc69 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -66,3 +66,30 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
 
return ret;
 }
+
+int edit_todo_list(unsigned flags)
+{
+   struct strbuf buf = STRBUF_INIT;
+   const char *todo_file = rebase_path_todo();
+
+   if (strbuf_read_file(, todo_file, 0) < 0)
+   return error_errno(_("could not read '%s'."), todo_file);
+
+   strbuf_stripspace(, 1);
+   if (write_message(buf.buf, buf.len, todo_file, 0)) {
+   strbuf_release();
+   return -1;
+   }
+
+   strbuf_release();
+
+   transform_todos(flags | TODO_LIST_SHORTEN_IDS);
+   append_todo_help(1, 0);
+
+   if (launch_sequence_editor(todo_file, NULL, NULL))
+   return -1;
+
+   

[GSoC][PATCH v8 06/20] rebase -i: rewrite setup_reflog_action() in C

[Alban Gruin]

This rewrites (the misnamed) setup_reflog_action() from shell to C. The
new version is called prepare_branch_to_be_rebased().

A new command is added to rebase--helper.c, “checkout-base”, as well as
a new flag, “verbose”, to avoid silencing the output of the checkout
operation called by checkout_base_commit().

The function `run_git_checkout()` will also be used in the next commit,
therefore its code is not part of `checkout_base_commit()`.

The shell version is then stripped in favour of a call to the helper.

As $GIT_REFLOG_ACTION is no longer set at the first call of
checkout_onto(), a call to comment_for_reflog() is added at the
beginning of this function.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--helper.c   |  7 ++-
 git-rebase--interactive.sh | 16 ++--
 sequencer.c| 30 ++
 sequencer.h|  2 ++
 4 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 731a64971d..0e76dadba6 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -18,7 +18,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO
+   ADD_EXEC, APPEND_TODO_HELP, EDIT_TODO, PREPARE_BRANCH
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -28,6 +28,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge 
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
 N_("keep original branch points of cousins")),
+   OPT__VERBOSE(, N_("be verbose")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -51,6 +52,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_CMDMODE(0, "edit-todo", ,
N_("edit the todo list during an interactive 
rebase"),
EDIT_TODO),
+   OPT_CMDMODE(0, "prepare-branch", ,
+   N_("prepare the branch to be rebased"), 
PREPARE_BRANCH),
OPT_END()
};
 
@@ -94,5 +97,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!append_todo_help(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
+   if (command == PREPARE_BRANCH && argc == 2)
+   return !!prepare_branch_to_be_rebased(, argv[1]);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2defe607f4..77e972bb6c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -72,6 +72,7 @@ collapse_todo_ids() {
 
 # Switch to the branch in $into and notify it in the reflog
 checkout_onto () {
+   comment_for_reflog start
GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
output git checkout $onto || die_abort "$(gettext "could not detach 
HEAD")"
git update-ref ORIG_HEAD $orig_head
@@ -119,19 +120,6 @@ initiate_action () {
esac
 }
 
-setup_reflog_action () {
-   comment_for_reflog start
-
-   if test ! -z "$switch_to"
-   then
-   GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
-   output git checkout "$switch_to" -- ||
-   die "$(eval_gettext "Could not checkout \$switch_to")"
-
-   comment_for_reflog start
-   fi
-}
-
 init_basic_state () {
orig_head=$(git rev-parse --verify HEAD) || die "$(gettext "No HEAD?")"
mkdir -p "$state_dir" || die "$(eval_gettext "Could not create 
temporary \$state_dir")"
@@ -211,7 +199,7 @@ git_rebase__interactive () {
return 0
fi
 
-   setup_reflog_action
+   git rebase--helper --prepare-branch "$switch_to" ${verbose:+--verbose}
init_basic_state
 
init_revisions_and_shortrevisions
diff --git a/sequencer.c b/sequencer.c
index 5919ad312e..d8a59c50f8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3285,6 +3285,36 @@ static const char *reflog_message(struct replay_opts 
*opts,
return buf.buf;
 }
 
+static int run_git_checkout(struct replay_opts *opts, const char *commit,
+   const char *action)
+{
+   struct child_process cmd = CHILD_PROCESS_INIT;
+
+   cmd.git_cmd = 1;
+
+   argv_array_push(, "checkout");
+   argv_array_push(, commit);
+   argv_array_pushf(_array, 

[GSoC][PATCH v8 02/20] rebase -i: rewrite append_todo_help() in C

[Alban Gruin]

This rewrites append_todo_help() from shell to C. It also incorporates
some parts of initiate_action() and complete_action() that also write
help texts to the todo file.

This also introduces the source file rebase-interactive.c. This file
will contain functions necessary for interactive rebase that are too
specific for the sequencer, and is part of libgit.a.

Two flags are added to rebase--helper.c: one to call
append_todo_help() (`--append-todo-help`), and another one to tell
append_todo_help() to write the help text suited for the edit-todo
mode (`--write-edit-todo`).

Finally, append_todo_help() is removed from git-rebase--interactive.sh
to use `rebase--helper --append-todo-help` instead.

Signed-off-by: Alban Gruin 
---
The changes are due to the rebase, no real changes otherwise.

 Makefile   |  1 +
 builtin/rebase--helper.c   | 11 --
 git-rebase--interactive.sh | 52 ++---
 rebase-interactive.c   | 68 ++
 rebase-interactive.h   |  6 
 5 files changed, 86 insertions(+), 52 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h

diff --git a/Makefile b/Makefile
index 13e1c52478..ff7f01634f 100644
--- a/Makefile
+++ b/Makefile
@@ -942,6 +942,7 @@ LIB_OBJS += quote.o
 LIB_OBJS += range-diff.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
+LIB_OBJS += rebase-interactive.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index f7c2a5fdc8..05e73e71d4 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -3,6 +3,7 @@
 #include "config.h"
 #include "parse-options.h"
 #include "sequencer.h"
+#include "rebase-interactive.h"
 
 static const char * const builtin_rebase_helper_usage[] = {
N_("git rebase--helper []"),
@@ -12,12 +13,12 @@ static const char * const builtin_rebase_helper_usage[] = {
 int cmd_rebase__helper(int argc, const char **argv, const char *prefix)
 {
struct replay_opts opts = REPLAY_OPTS_INIT;
-   unsigned flags = 0, keep_empty = 0, rebase_merges = 0;
+   unsigned flags = 0, keep_empty = 0, rebase_merges = 0, write_edit_todo 
= 0;
int abbreviate_commands = 0, rebase_cousins = -1;
enum {
CONTINUE = 1, ABORT, MAKE_SCRIPT, SHORTEN_OIDS, EXPAND_OIDS,
CHECK_TODO_LIST, SKIP_UNNECESSARY_PICKS, REARRANGE_SQUASH,
-   ADD_EXEC
+   ADD_EXEC, APPEND_TODO_HELP
} command = 0;
struct option options[] = {
OPT_BOOL(0, "ff", _ff, N_("allow fast-forward")),
@@ -27,6 +28,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
OPT_BOOL(0, "rebase-merges", _merges, N_("rebase merge 
commits")),
OPT_BOOL(0, "rebase-cousins", _cousins,
 N_("keep original branch points of cousins")),
+   OPT_BOOL(0, "write-edit-todo", _edit_todo,
+N_("append the edit-todo message to the todo-list")),
OPT_CMDMODE(0, "continue", , N_("continue rebase"),
CONTINUE),
OPT_CMDMODE(0, "abort", , N_("abort rebase"),
@@ -45,6 +48,8 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
N_("rearrange fixup/squash lines"), REARRANGE_SQUASH),
OPT_CMDMODE(0, "add-exec-commands", ,
N_("insert exec commands in todo list"), ADD_EXEC),
+   OPT_CMDMODE(0, "append-todo-help", ,
+   N_("insert the help in the todo list"), 
APPEND_TODO_HELP),
OPT_END()
};
 
@@ -84,5 +89,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
return !!rearrange_squash();
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
+   if (command == APPEND_TODO_HELP && argc == 1)
+   return !!append_todo_help(write_edit_todo, keep_empty);
usage_with_options(builtin_rebase_helper_usage, options);
 }
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded2137..94c23a7af2 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -39,38 +39,6 @@ comment_for_reflog () {
esac
 }
 
-append_todo_help () {
-   gettext "
-Commands:
-p, pick  = use commit
-r, reword  = use commit, but edit the commit message
-e, edit  = use commit, but stop for amending
-s, squash  = use commit, but meld into previous commit
-f, fixup  = like \"squash\", but discard this commit's log message
-x, exec  = run command (the rest of the line) using shell
-d, drop  = remove commit
-l, label  = label current HEAD with a name
-t, reset  = reset HEAD to a label
-m, merge [-C  | -c ]  [# ]
-.   create a merge commit using the original merge commit's
-.

[GSoC][PATCH v8 08/20] sequencer: refactor append_todo_help() to write its message to a buffer

[Alban Gruin]

This refactors append_todo_help() to write its message to a buffer
instead of the todo-list.  This is needed for the rewrite of
complete_action(), which will come after the next commit.

As rebase--helper still needs the file manipulation part of
append_todo_help(), it is extracted to a temporary function,
append_todo_help_to_file().  This function will go away after the
rewrite of complete_action().

Signed-off-by: Alban Gruin 
---
No changes since v7.

 builtin/rebase--helper.c |  2 +-
 rebase-interactive.c | 43 
 rebase-interactive.h |  4 +++-
 3 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/builtin/rebase--helper.c b/builtin/rebase--helper.c
index 7d9426d23c..313092c465 100644
--- a/builtin/rebase--helper.c
+++ b/builtin/rebase--helper.c
@@ -97,7 +97,7 @@ int cmd_rebase__helper(int argc, const char **argv, const 
char *prefix)
if (command == ADD_EXEC && argc == 2)
return !!sequencer_add_exec_commands(argv[1]);
if (command == APPEND_TODO_HELP && argc == 1)
-   return !!append_todo_help(0, keep_empty);
+   return !!append_todo_help_to_file(0, keep_empty);
if (command == EDIT_TODO && argc == 1)
return !!edit_todo_list(flags);
if (command == PREPARE_BRANCH && argc == 2)
diff --git a/rebase-interactive.c b/rebase-interactive.c
index 3f9468fc69..4a9a10eff4 100644
--- a/rebase-interactive.c
+++ b/rebase-interactive.c
@@ -4,11 +4,9 @@
 #include "sequencer.h"
 #include "strbuf.h"
 
-int append_todo_help(unsigned edit_todo, unsigned keep_empty)
+void append_todo_help(unsigned edit_todo, unsigned keep_empty,
+ struct strbuf *buf)
 {
-   struct strbuf buf = STRBUF_INIT;
-   FILE *todo;
-   int ret;
const char *msg = _("\nCommands:\n"
 "p, pick  = use commit\n"
 "r, reword  = use commit, but edit the commit message\n"
@@ -26,11 +24,7 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
 "\n"
 "These lines can be re-ordered; they are executed from top to bottom.\n");
 
-   todo = fopen_or_warn(rebase_path_todo(), "a");
-   if (!todo)
-   return 1;
-
-   strbuf_add_commented_lines(, msg, strlen(msg));
+   strbuf_add_commented_lines(buf, msg, strlen(msg));
 
if (get_missing_commit_check_level() == MISSING_COMMIT_CHECK_ERROR)
msg = _("\nDo not remove any line. Use 'drop' "
@@ -39,7 +33,7 @@ int append_todo_help(unsigned edit_todo, unsigned keep_empty)
msg = _("\nIf you remove a line here "
 "THAT COMMIT WILL BE LOST.\n");
 
-   strbuf_add_commented_lines(, msg, strlen(msg));
+   strbuf_add_commented_lines(buf, msg, strlen(msg));
 
if (edit_todo)
msg = _("\nYou are editing the todo file "
@@ -50,12 +44,25 @@ int append_todo_help(unsigned edit_todo, unsigned 
keep_empty)
msg = _("\nHowever, if you remove everything, "
"the rebase will be aborted.\n\n");
 
-   strbuf_add_commented_lines(, msg, strlen(msg));
+   strbuf_add_commented_lines(buf, msg, strlen(msg));
 
if (!keep_empty) {
msg = _("Note that empty commits are commented out");
-   strbuf_add_commented_lines(, msg, strlen(msg));
+   strbuf_add_commented_lines(buf, msg, strlen(msg));
}
+}
+
+int append_todo_help_to_file(unsigned edit_todo, unsigned keep_empty)
+{
+   struct strbuf buf = STRBUF_INIT;
+   FILE *todo;
+   int ret;
+
+   todo = fopen_or_warn(rebase_path_todo(), "a");
+   if (!todo)
+   return -1;
+
+   append_todo_help(edit_todo, keep_empty, );
 
ret = fputs(buf.buf, todo);
if (ret < 0)
@@ -84,7 +91,17 @@ int edit_todo_list(unsigned flags)
strbuf_release();
 
transform_todos(flags | TODO_LIST_SHORTEN_IDS);
-   append_todo_help(1, 0);
+
+   if (strbuf_read_file(, todo_file, 0) < 0)
+   return error_errno(_("could not read '%s'."), todo_file);
+
+   append_todo_help(1, 0, );
+   if (write_message(buf.buf, buf.len, todo_file, 0)) {
+   strbuf_release();
+   return -1;
+   }
+
+   strbuf_release();
 
if (launch_sequence_editor(todo_file, NULL, NULL))
return -1;
diff --git a/rebase-interactive.h b/rebase-interactive.h
index 155219e742..d33f3176b7 100644
--- a/rebase-interactive.h
+++ b/rebase-interactive.h
@@ -1,7 +1,9 @@
 #ifndef REBASE_INTERACTIVE_H
 #define REBASE_INTERACTIVE_H
 
-int append_todo_help(unsigned edit_todo, unsigned keep_empty);
+void append_todo_help(unsigned edit_todo, unsigned keep_empty,
+ struct strbuf *buf);
+int append_todo_help_to_file(unsigned edit_todo, unsigned keep_empty);
 int edit_todo_list(unsigned flags);
 
 #endif
-- 
2.19.0

[GSoC][PATCH v8 03/20] editor: add a function to launch the sequence editor

[Alban Gruin]

As part of the rewrite of interactive rebase, the sequencer will need to
open the sequence editor to allow the user to edit the todo list.
Instead of duplicating the existing launch_editor() function, this
refactors it to a new function, launch_specified_editor(), which takes
the editor as a parameter, in addition to the path, the buffer and the
environment variables.  launch_sequence_editor() is then added to launch
the sequence editor.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 cache.h  |  1 +
 editor.c | 27 +--
 strbuf.h |  2 ++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index d508f3d4f8..a976e26a02 100644
--- a/cache.h
+++ b/cache.h
@@ -1482,6 +1482,7 @@ extern const char *fmt_name(const char *name, const char 
*email);
 extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
+extern const char *git_sequence_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 extern int is_terminal_dumb(void);
 extern int git_ident_config(const char *, const char *, void *);
diff --git a/editor.c b/editor.c
index 9a9b4e12d1..c985eee1f9 100644
--- a/editor.c
+++ b/editor.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "config.h"
 #include "strbuf.h"
 #include "run-command.h"
 #include "sigchain.h"
@@ -34,10 +35,21 @@ const char *git_editor(void)
return editor;
 }
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const 
*env)
+const char *git_sequence_editor(void)
 {
-   const char *editor = git_editor();
+   const char *editor = getenv("GIT_SEQUENCE_EDITOR");
+
+   if (!editor)
+   git_config_get_string_const("sequence.editor", );
+   if (!editor)
+   editor = git_editor();
 
+   return editor;
+}
+
+static int launch_specified_editor(const char *editor, const char *path,
+  struct strbuf *buffer, const char *const 
*env)
+{
if (!editor)
return error("Terminal is dumb, but EDITOR unset");
 
@@ -95,3 +107,14 @@ int launch_editor(const char *path, struct strbuf *buffer, 
const char *const *en
return error_errno("could not read file '%s'", path);
return 0;
 }
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const 
*env)
+{
+   return launch_specified_editor(git_editor(), path, buffer, env);
+}
+
+int launch_sequence_editor(const char *path, struct strbuf *buffer,
+  const char *const *env)
+{
+   return launch_specified_editor(git_sequence_editor(), path, buffer, 
env);
+}
diff --git a/strbuf.h b/strbuf.h
index 60a35aef16..66da9822fd 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -575,6 +575,8 @@ extern void strbuf_add_unique_abbrev(struct strbuf *sb,
  * file's contents are not read into the buffer upon completion.
  */
 extern int launch_editor(const char *path, struct strbuf *buffer, const char 
*const *env);
+extern int launch_sequence_editor(const char *path, struct strbuf *buffer,
+ const char *const *env);
 
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char 
*buf, size_t size);
 
-- 
2.19.0

[GSoC][PATCH v8 05/20] sequencer: add a new function to silence a command, except if it fails

[Alban Gruin]

This adds a new function, run_command_silent_on_success(), to
redirect the stdout and stderr of a command to a strbuf, and then to run
that command. This strbuf is printed only if the command fails. It is
functionnaly similar to output() from git-rebase.sh.

run_git_commit() is then refactored to use of
run_command_silent_on_success().

Signed-off-by: Alban Gruin 
---
The changes are due to the rebase, no real changes otherwise.

 sequencer.c | 51 +--
 1 file changed, 25 insertions(+), 26 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c24d37dfb1..5919ad312e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -804,6 +804,23 @@ N_("you have staged changes in your working tree\n"
 #define VERIFY_MSG  (1<<4)
 #define CREATE_ROOT_COMMIT (1<<5)
 
+static int run_command_silent_on_success(struct child_process *cmd)
+{
+   struct strbuf buf = STRBUF_INIT;
+   int rc;
+
+   cmd->stdout_to_stderr = 1;
+   rc = pipe_command(cmd,
+ NULL, 0,
+ NULL, 0,
+ , 0);
+
+   if (rc)
+   fputs(buf.buf, stderr);
+   strbuf_release();
+   return rc;
+}
+
 /*
  * If we are cherry-pick, and if the merge did not result in
  * hand-editing, we will hit this commit and inherit the original
@@ -865,18 +882,11 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
 
cmd.git_cmd = 1;
 
-   if (is_rebase_i(opts)) {
-   if (!(flags & EDIT_MSG)) {
-   cmd.stdout_to_stderr = 1;
-   cmd.err = -1;
-   }
+   if (is_rebase_i(opts) && read_env_script(_array)) {
+   const char *gpg_opt = gpg_sign_opt_quoted(opts);
 
-   if (read_env_script(_array)) {
-   const char *gpg_opt = gpg_sign_opt_quoted(opts);
-
-   return error(_(staged_changes_advice),
-gpg_opt, gpg_opt);
-   }
+   return error(_(staged_changes_advice),
+gpg_opt, gpg_opt);
}
 
argv_array_push(, "commit");
@@ -906,21 +916,10 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
if (!(flags & EDIT_MSG))
argv_array_push(, "--allow-empty-message");
 
-   if (cmd.err == -1) {
-   /* hide stderr on success */
-   struct strbuf buf = STRBUF_INIT;
-   int rc = pipe_command(,
- NULL, 0,
- /* stdout is already redirected */
- NULL, 0,
- , 0);
-   if (rc)
-   fputs(buf.buf, stderr);
-   strbuf_release();
-   return rc;
-   }
-
-   return run_command();
+   if (is_rebase_i(opts) && !(flags & EDIT_MSG))
+   return run_command_silent_on_success();
+   else
+   return run_command();
 }
 
 static int rest_is_empty(const struct strbuf *sb, int start)
-- 
2.19.0

[GSoC][PATCH v8 00/20] rebase -i: rewrite in C

[Alban Gruin]

This patch series rewrite the interactive rebase from shell to C.

The v7 was based on ffc6fa0e39 ("Fourth batch for 2.19 cycle",
2018-07-24), but this series is based on fe8321ec05 ("Second batch post
2.19", 2017-09-24) due to some conflicts.

Changes since v7:

 - [17/20] The optionnal parameter of `-S' was mandatory in
   rebase--builtin2, making it abort when a signing key is provided to
   `user.signingkey' and `commit.gpgsign' is set to true.  First
   reported on github[0], fixed by Johannes Schindelin.

 - [2, 5, 19, 20/20] Changes due to the rebase.

 - [9, 11/20] Conflict solving.

Alban Gruin (20):
  sequencer: make three functions and an enum from sequencer.c public
  rebase -i: rewrite append_todo_help() in C
  editor: add a function to launch the sequence editor
  rebase -i: rewrite the edit-todo functionality in C
  sequencer: add a new function to silence a command, except if it fails
  rebase -i: rewrite setup_reflog_action() in C
  rebase -i: rewrite checkout_onto() in C
  sequencer: refactor append_todo_help() to write its message to a
buffer
  sequencer: change the way skip_unnecessary_picks() returns its result
  t3404: todo list with commented-out commands only aborts
  rebase -i: rewrite complete_action() in C
  rebase -i: remove unused modes and functions
  rebase -i: implement the logic to initialize $revisions in C
  rebase -i: rewrite the rest of init_revisions_and_shortrevisions() in
C
  rebase -i: rewrite write_basic_state() in C
  rebase -i: rewrite init_basic_state() in C
  rebase -i: implement the main part of interactive rebase as a builtin
  rebase--interactive2: rewrite the submodes of interactive rebase in C
  rebase -i: remove git-rebase--interactive.sh
  rebase -i: move rebase--helper modes to rebase--interactive

 .gitignore |   1 -
 Makefile   |   5 +-
 builtin.h  |   2 +-
 builtin/rebase--helper.c   |  88 -
 builtin/rebase--interactive.c  | 271 
 cache.h|   1 +
 editor.c   |  27 ++-
 git-rebase--interactive.sh | 283 -
 git-rebase--preserve-merges.sh |  10 +-
 git-rebase.sh  |  47 -
 git.c  |   2 +-
 rebase-interactive.c   |  90 ++
 rebase-interactive.h   |   8 +
 sequencer.c| 320 +++--
 sequencer.h|  22 ++-
 strbuf.h   |   2 +
 t/t3404-rebase-interactive.sh  |  10 ++
 17 files changed, 740 insertions(+), 449 deletions(-)
 delete mode 100644 builtin/rebase--helper.c
 create mode 100644 builtin/rebase--interactive.c
 delete mode 100644 git-rebase--interactive.sh
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h

-- 
2.19.0

[GSoC][PATCH v8 01/20] sequencer: make three functions and an enum from sequencer.c public

[Alban Gruin]

This makes rebase_path_todo(), get_missing_commit_check_level(),
write_message() and the enum check_level accessible outside sequencer.c,
renames check_level to missing_commit_check_level, and prefixes its
value names by MISSING_COMMIT_ to avoid namespace pollution.

This function and this enum will eventually be moved to
rebase-interactive.c and become static again, so no special attention
was given to the naming.

This will be needed for the rewrite of append_todo_help() from shell to
C, as it will be in a new library source file, rebase-interactive.c.

Signed-off-by: Alban Gruin 
---
No changes since v7.

 sequencer.c | 26 +++---
 sequencer.h | 11 +++
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ddb41a62d9..c24d37dfb1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -53,7 +53,7 @@ static GIT_PATH_FUNC(rebase_path, "rebase-merge")
  * the lines are processed, they are removed from the front of this
  * file and written to the tail of 'done'.
  */
-static GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
+GIT_PATH_FUNC(rebase_path_todo, "rebase-merge/git-rebase-todo")
 /*
  * The rebase command lines that have already been processed. A line
  * is moved here when it is first handled, before any associated user
@@ -377,8 +377,8 @@ static void print_advice(int show_hint, struct replay_opts 
*opts)
}
 }
 
-static int write_message(const void *buf, size_t len, const char *filename,
-int append_eol)
+int write_message(const void *buf, size_t len, const char *filename,
+ int append_eol)
 {
struct lock_file msg_file = LOCK_INIT;
 
@@ -4422,24 +4422,20 @@ int transform_todos(unsigned flags)
return i;
 }
 
-enum check_level {
-   CHECK_IGNORE = 0, CHECK_WARN, CHECK_ERROR
-};
-
-static enum check_level get_missing_commit_check_level(void)
+enum missing_commit_check_level get_missing_commit_check_level(void)
 {
const char *value;
 
if (git_config_get_value("rebase.missingcommitscheck", ) ||
!strcasecmp("ignore", value))
-   return CHECK_IGNORE;
+   return MISSING_COMMIT_CHECK_IGNORE;
if (!strcasecmp("warn", value))
-   return CHECK_WARN;
+   return MISSING_COMMIT_CHECK_WARN;
if (!strcasecmp("error", value))
-   return CHECK_ERROR;
+   return MISSING_COMMIT_CHECK_ERROR;
warning(_("unrecognized setting %s for option "
  "rebase.missingCommitsCheck. Ignoring."), value);
-   return CHECK_IGNORE;
+   return MISSING_COMMIT_CHECK_IGNORE;
 }
 
 define_commit_slab(commit_seen, unsigned char);
@@ -4451,7 +4447,7 @@ define_commit_slab(commit_seen, unsigned char);
  */
 int check_todo_list(void)
 {
-   enum check_level check_level = get_missing_commit_check_level();
+   enum missing_commit_check_level check_level = 
get_missing_commit_check_level();
struct strbuf todo_file = STRBUF_INIT;
struct todo_list todo_list = TODO_LIST_INIT;
struct strbuf missing = STRBUF_INIT;
@@ -4468,7 +4464,7 @@ int check_todo_list(void)
advise_to_edit_todo = res =
parse_insn_buffer(todo_list.buf.buf, _list);
 
-   if (res || check_level == CHECK_IGNORE)
+   if (res || check_level == MISSING_COMMIT_CHECK_IGNORE)
goto leave_check;
 
/* Mark the commits in git-rebase-todo as seen */
@@ -4503,7 +4499,7 @@ int check_todo_list(void)
if (!missing.len)
goto leave_check;
 
-   if (check_level == CHECK_ERROR)
+   if (check_level == MISSING_COMMIT_CHECK_ERROR)
advise_to_edit_todo = res = 1;
 
fprintf(stderr,
diff --git a/sequencer.h b/sequencer.h
index c986bc8251..579de9127b 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -8,6 +8,7 @@ struct commit;
 
 const char *git_path_commit_editmsg(void);
 const char *git_path_seq_dir(void);
+const char *rebase_path_todo(void);
 
 #define APPEND_SIGNOFF_DEDUP (1u << 0)
 
@@ -62,6 +63,15 @@ struct replay_opts {
 };
 #define REPLAY_OPTS_INIT { .action = -1, .current_fixups = STRBUF_INIT }
 
+enum missing_commit_check_level {
+   MISSING_COMMIT_CHECK_IGNORE = 0,
+   MISSING_COMMIT_CHECK_WARN,
+   MISSING_COMMIT_CHECK_ERROR
+};
+
+int write_message(const void *buf, size_t len, const char *filename,
+ int append_eol);
+
 /* Call this to setup defaults before parsing command line options */
 void sequencer_init_config(struct replay_opts *opts);
 int sequencer_pick_revisions(struct replay_opts *opts);
@@ -84,6 +94,7 @@ int sequencer_make_script(FILE *out, int argc, const char 
**argv,
 
 int sequencer_add_exec_commands(const char *command);
 int transform_todos(unsigned flags);
+enum missing_commit_check_level get_missing_commit_check_level(void);
 int check_todo_list(void);
 int skip_unnecessary_picks(void);
 int rearrange_squash(void);
-- 

Re: [PATCH] Documentation/CodingGuidelines: How to document new APIs

[Junio C Hamano]

Stefan Beller  writes:

> There are different opinions on how to document an API properly.
> Discussion turns out nobody disagrees with documenting new APIs on the
> function level in the header file and high level concepts in

Looks conditionally good ;-) Thanks for keeping the ball rolling.

I didn't get an impression that "next to implementation" vs "in
header to force people write clearly" was totally settled.  I'd wait
until Ævar says something on this ;-)


> Documentation/technical, so let's state that in the guidelines.
>
> Signed-off-by: Stefan Beller 
> ---
>
>  This is how I would approach the documentation patch.
>  
>  Thanks,
>  Stefan
>  
>  Documentation/CodingGuidelines | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>  
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 48aa4edfbd..15bfb8bbb8 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -358,7 +358,10 @@ For C programs:
> string_list for sorted string lists, a hash map (mapping struct
> objects) named "struct decorate", amongst other things.
>  
> - - When you come up with an API, document it.
> + - When you come up with an API, document the functions in the header
> +   and highlevel concepts in Documentation/technical/. Note that this
> +   directory still contains function level documentation as historically
> +   everything was documented there.
>  
>   - The first #include in C files, except in platform specific compat/
> implementations, must be either "git-compat-util.h", "cache.h" or

Re: [RFC PATCH v2 4/4] fetch: do not list refs if fetching only hashes

[Stefan Beller]

On Thu, Sep 27, 2018 at 12:24 PM Jonathan Tan  wrote:
>
> If only hash literals are given on a "git fetch" command-line, tag
> following is not requested, and the fetch is done using protocol v2, a
> list of refs is not required from the remote. Therefore, optimize by
> invoking transport_get_remote_refs() only if we need the refs.
>

Makes sense

> +
> +   /*
> +* We can avoid listing refs if all of them are exact
> +* OIDs
> +*/
> +   must_list_refs = 0;
> +   for (i = 0; i < rs->nr; i++) {
> +   if (!rs->items[i].exact_sha1) {
> +   must_list_refs = 1;
> +   break;
> +   }
> +   }

This seems to be a repeat pattern, Is it worth it to encapsulate it
as a function in transport or refs?

  int must_list_refs(struct ref **to_fetch)
  {
// body as the loop above
  }

Re: [RFC PATCH v2 2/4] transport: do not list refs if possible

[Stefan Beller]

On Thu, Sep 27, 2018 at 12:24 PM Jonathan Tan  wrote:

>
> +test_expect_success 'when dynamically fetching missing object, do not list 
> refs' '
> +   cat trace &&

leftover debug cat?

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Rafael Ascensão]

On Thu, Sep 27, 2018 at 03:35:59PM -0400, Jeff King wrote:
> On Thu, Sep 27, 2018 at 08:28:04PM +0100, Rafael Ascensão wrote:
> > But if we're open to change how branches are displayed maybe a config
> > option like branch.format (probably not the best name choice) that can
> > be set to the 'for-each-ref --format' syntax would be way more flexible.
> 
> We have that already, don't we?
>

git branch has --format, but there's no way (at least to my knowledge)
to define a value in gitconfig to be used by $git branch.

Having branch --format available, making an alias is a possible route.
(Either by wrapping branch --format or for-each-ref itself). But I was
referring to changing the output of the default $git branch;

[PATCH] Documentation/CodingGuidelines: How to document new APIs

[Stefan Beller]

There are different opinions on how to document an API properly.
Discussion turns out nobody disagrees with documenting new APIs on the
function level in the header file and high level concepts in
Documentation/technical, so let's state that in the guidelines.

Signed-off-by: Stefan Beller 
---

 This is how I would approach the documentation patch.
 
 Thanks,
 Stefan
 
 Documentation/CodingGuidelines | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)
 
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 48aa4edfbd..15bfb8bbb8 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -358,7 +358,10 @@ For C programs:
string_list for sorted string lists, a hash map (mapping struct
objects) named "struct decorate", amongst other things.
 
- - When you come up with an API, document it.
+ - When you come up with an API, document the functions in the header
+   and highlevel concepts in Documentation/technical/. Note that this
+   directory still contains function level documentation as historically
+   everything was documented there.
 
  - The first #include in C files, except in platform specific compat/
implementations, must be either "git-compat-util.h", "cache.h" or
-- 
2.19.0

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Junio C Hamano]

Jeff King  writes:

> On Thu, Sep 27, 2018 at 03:35:59PM -0400, Jeff King wrote:
>
>> now, we could do:
>> 
>>   %(if)%(HEAD)%(then)* %(color:bold green)
>>   %(else)%(if)%(worktree)%(then)+ %(color:green)
>>   %(else)  %(end)%(end)
>> 
>> (respecting the user's color config, of course, rather than hard-coded
>> colors).
>> 
>> Trying that out, though, I'm not sure if we properly support nested
>> if's. That might be a bug we have to fix first.
>
> Sorry, false alarm. I just had a typo in my format.
>
> This seems to work with the patch I posted earlier:
>
>   git for-each-ref \
> --format='%(if)%(HEAD)%(then)* %(color:bold 
> green)%(else)%(if)%(worktree)%(then)+ %(color:green)%(else) 
> %(end)%(end)%(refname)' \
>   refs/heads
>
> It sure would be nice if there was a way to insert line breaks without
> impacting the output. ;)
>
> -Peff

I envy all of you who seem to have had a lot of fun while I was
doing something else (which were rather boring but still needed
precision--my least favorite kind of task X-<).

The only comment I have is that I strongly suspect we will regret if
we used an overly bland "worktree" to a rather narrrow "is this ref
checked out in any worktree?" when we notice we want to learn other
things that are related to "worktree".  Other than that, very nicely
done.

Re: [PATCH v3 1/5] CodingGuidelines: add shell piping guidelines

[SZEDER Gábor]

On Tue, Sep 25, 2018 at 02:58:08PM -0700, Matthew DeVore wrote:
> Here is the new commit with updated message (I will wait for a day or
> two before I send a reroll):
> 
> Documentation: add shell guidelines
> 
> Add the following guideline to Documentation/CodingGuidelines:
> 
> &&, ||, and | should appear at the end of lines, not the
> beginning, and the \ line continuation character should be
> omitted
> 
> And the following to t/README (since it is specific to writing tests):
> 
> pipes and $(git ...) should be avoided when they swallow exit
> codes of Git processes
> 
> Signed-off-by: Matthew DeVore 
> 
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 48aa4edfb..3d2cfea9b 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -118,6 +118,24 @@ For shell scripts specifically (not exhaustive):
>  do this
>  fi
> 
> + - If a command sequence joined with && or || or | spans multiple
> +   lines, put each command on a separate line and put && and || and |
> +   operators at the end of each line, rather than the start. This
> +   means you don't need to use \ to join lines, since the above
> +   operators imply the sequence isn't finished.
> +
> +(incorrect)
> +grep blob verify_pack_result \
> +| awk -f print_1.awk \
> +| sort >actual &&
> +...
> +
> +(correct)
> +grep blob verify_pack_result |
> +awk -f print_1.awk |
> +sort >actual &&
> +...
> +
>   - We prefer "test" over "[ ... ]".
> 
>   - We do not write the noiseword "function" in front of shell
> @@ -163,7 +181,6 @@ For shell scripts specifically (not exhaustive):
> 
> does not have such a problem.
> 
> -
>  For C programs:
> 
>   - We use tabs to indent, and interpret tabs as taking up to
> diff --git a/t/README b/t/README
> index 9028b47d9..3e28b72c4 100644
> --- a/t/README
> +++ b/t/README
> @@ -461,6 +461,32 @@ Don't:
> platform commands; just use '! cmd'.  We are not in the business
> of verifying that the world given to us sanely works.
> 
> + - Use Git upstream in the non-final position in a piped chain, as in:

Note the starting upper case 'U'.

> +
> + git -C repo ls-files |
> + xargs -n 1 basename |
> + grep foo
> +
> +   which will discard git's exit code and may mask a crash. In the
> +   above example, all exit codes are ignored except grep's.
> +
> +   Instead, write the output of that command to a temporary
> +   file with ">" or assign it to a variable with "x=$(git ...)" rather
> +   than pipe it.
> +
> + - Use command substitution in a way that discards git's exit code.

'U' again.

> +   When assigning to a variable, the exit code is not discarded, e.g.:
> +
> + x=$(git cat-file -p $sha) &&
> + ...
> +
> +   is OK because a crash in "git cat-file" will cause the "&&" chain
> +   to fail, but:
> +
> + test_cmp expect $(git cat-file -p $sha)
> +
> +   is not OK and a crash in git could go undetected.

Well, this is not OK indeed, because it doesn't make any sense in the
first place :)  'test_cmp' requires two paths as argumens, but the
output of 'git cat-file -p' is the whole _content_ of the given object.

>   - use perl without spelling it as "$PERL_PATH". This is to help our

Note the starting lower case 'u'.

This is because these are the continuation of the "Don't:" some lines
earlier, so your new points should start with a lower case 'u' as
well.


Sidenote: I think we should consider reformatting this whole section
as:

  - Don't do this.
  - Don't do that.

because it grew so much that when I look at the last points, then that
starting "Don't:" has already scrolled out of my screen.

> friends on Windows where the platform Perl often adds CR before
> the end of line, and they bundle Git with a version of Perl that
> 
> 
> >
> >
> > These last two points, however, are specific to test scripts,
> > therefore I think they would be better placed in 't/README', where the
> > rest of the test-specific guidelines are.
> >
> > >  For C programs:
> > >
> > > --
> > > 2.19.0.444.g18242da7ef-goog
> > >

Re: [RFC PATCH v2 0/4] Avoid ls-refs when possible in protocol v2

[Junio C Hamano]

Jonathan Tan  writes:

> To answer Junio's questions in [1], I think it's best to include the
> full patch set that I'm developing, so here it is. The original patch is
> now patch 3 of this set.

Yeah, taking it out of context did make its purpose fuzzy.  Without
the other patches in the series that set the overall direction of
reducing ls-refs, it was unclear why some transports are marked to
specifically want to automatically call transport_get_remote_refs().
With these surrounding changes, "we call it as needed, because an
earlier step may not have called it" starts to sound quite sensible.

Re: t7005-editor.sh failure

[SZEDER Gábor]

On Wed, Sep 26, 2018 at 12:16:16PM -0700, Junio C Hamano wrote:
> Junio C Hamano  writes:

> > I quote >"$file" (but not var=$var) because the CodingGuidelines
> > tells me to:
> >
> >  - Redirection operators should be written with space before, but no
> >space after them.  In other words, write 'echo test >"$file"'
> >instead of 'echo test> $file' or 'echo test > $file'.  Note that
> >even though it is not required by POSIX to double-quote the
> >redirection target in a variable (as shown above), our code does so
> >because some versions of bash issue a warning without the quotes.
> >
> > ;-)

Oh, indeed, I didn't notice that.

> Subject: t7005: make sure it passes under /bin/bash
> 
> In POSIX.1 compliant shells, you should be able to use a variable
> reference without quoting for the target of the redirection, e.g.
> 
>   echo foo >$file
>   echo bar >$1
> 
> without fear of substitution of $file getting split at $IFS.
> However, some versions of bash throws a warning, especially when

I would say it's an error, not a warning.

Regarding the "some versions", it's unclear when Bash started to apply
word splitting to the filename in redirection.  The changelog of
version 2.04-beta2 contains the following entry:

  When running in POSIX.2 mode, bash no longer performs word
  splitting on the expanded value of the word supplied as the filename
  argument to redirection operators.

so it must have started earlier, but I found further no sign of it in
the changelog.  In its man page the first ever mention of word
splitting affecting redirections came only later, in version 2.04.

v2.04 was release in 2000, so it's bordering on "since forever".


> they are invoked as /bin/bash (not as /bin/sh).  Those who build
> with SHELL_PATH=/bin/bash and run t/t7005-editor.sh triggers an

The grammar here confused me a bit...  maybe s/triggers/trigger/ ?

> unnecessary failure due to this issue.
> 
> Fix it by making sure that the generated "editor" script quotes the
> target of redirection.  
> 
> While at it, update the way to creatd these scripts to use the
> write_script wrapper, so that we do not have to worry about writing
> the she-bang line and making the result executable.
> 
> Reported-by: Alexander Pyhalov 
> Suggested-by: SZEDER Gábor 
> Signed-off-by: Junio C Hamano 
> ---
>  t/t7005-editor.sh | 9 -
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/t/t7005-editor.sh b/t/t7005-editor.sh
> index b2ca77b338..b0c4cc4ca0 100755
> --- a/t/t7005-editor.sh
> +++ b/t/t7005-editor.sh
> @@ -20,11 +20,9 @@ fi
>  
>  for i in GIT_EDITOR core_editor EDITOR VISUAL $vi
>  do
> - cat >e-$i.sh <<-EOF
> - #!$SHELL_PATH
> + write_script "e-$i.sh" <<-EOF

This can't be <<-\EOF ...

>   echo "Edited by $i" >"\$1"

... because here we have to expand $i, and, therefore, we need both
double-quotes and \-escaping for $1.  Ok.

>   EOF
> - chmod +x e-$i.sh
>  done
>  
>  if ! test -z "$vi"
> @@ -112,8 +110,9 @@ do
>  done
>  
>  test_expect_success 'editor with a space' '
> - echo "echo space >\$1" >"e space.sh" &&
> - chmod a+x "e space.sh" &&
> + write_script "e space.sh" <<-\EOF &&

But here it can be <<-\EOF ...

> + echo space >"$1"

... so here we don't need the \-escaping.  Good.

> + EOF
>   GIT_EDITOR="./e\ space.sh" git commit --amend &&
>   test space = "$(git show -s --pretty=format:%s)"
>  

Re: [PATCH 0/2] negotiator: improve recent behavior + docs

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 27 2018, Jonathan Tan wrote:

>> > If you wanted to do this, it seems better to me to just declare a "null"
>> > negotiation algorithm that does not perform any negotiation at all.
>>
>> I think such an algorithm is a good idea in general, especially for
>> testing, and yeah, maybe that's the best way out of this, i.e. to do:
>>
>> if git rev-parse {}/HEAD 2>/dev/null
>> then
>> git fetch --negotiation-tip={}/HEAD {}
>> else
>> git -c fetch.negotiationAlgorithm=null fetch {}
>> fi
>>
>> Would such an algorithm be added by overriding default.c's add_tip
>> function to never add anything by calling default_negotiator_init()
>> followed by null_negotiator_init(), which would only override add_tip?
>> (yay C OO)
>>
>> If so from fetch-pack.c it looks like there may be the limitation on the
>> interface that the negotiator can't exit early (in
>> fetch-pack.c:mark_tips). But I've just skimmed this, so maybe I've
>> missed something.
>
> (I was reminded to reply to this offlist - sorry for the late reply.)
>
> I think too many things need to be replaced (known_common, add_tip, and
> ack all need to do nothing), so it's best to start from scratch. That
> way, we also don't need to deal with the subtleties of C OO :-)
>
>> Also, looks like because of the current interface =null and
>> --negotiation-tip=* would (somewhat confusingly) do a "real" negotiation
>> if done that way, since it'll bypass the API and insert tips for it to
>> negotiate, but it looks like overriding next() will get around that.
>
> If you do it as I suggest (in particular, add_tip doing nothing) then
> there is the opposite problem that it won't be easy to inform the user
> that --negotiation-tip does nothing in this case. Maybe there needs to
> be an "accepts_tips" field in struct fetch_negotiator that, if false,
> means that custom tips (or any tips) are not accepted, allowing the
> caller of the negotiator to print a warning message in this case.

Thanks, yeah it seems the interface would need to be tweaked for such a
"null" negotiator.

Some more general questions (which I can turn into docs once I
understand this). If I run this, as a testcase for two random repos
where I "fetch" an unrelated one and use the first ever commit to
git.git as an alias for this "null" negotiatior, i.e. "just present this
one commit":

(
rm -rf /tmp/git &&
git clone https://github.com/git/git.git /tmp/git &&
cd /tmp/git &&
git remote add gitlab-shell 
https://github.com/cr-marcstevens/sha1collisiondetection &&
GIT_TRACE_PACKET=/tmp/git/packet.trace git fetch 
--negotiation-tip=$(git log --reverse|head -n 1|cut -d ' ' -f2) gitlab-shell &&
grep -c "fetch-pack> have" /tmp/git/packet.trace
)

I get:

warning: Ignoring --negotiation-tip because the protocol does not support 
it.

And the grep -c shows we tried to present 55170 commits in "have" lines
to the server. Now, change that to SSH and all is well:

(
rm -rf /tmp/git &&
git clone g...@github.com:git/git.git /tmp/git &&
cd /tmp/git &&
git remote add gitlab-shell 
g...@github.com:cr-marcstevens/sha1collisiondetection &&
GIT_TRACE_PACKET=/tmp/git/packet.trace git fetch 
--negotiation-tip=$(git log --reverse|head -n 1|cut -d ' ' -f2) gitlab-shell &&
grep -c "fetch-pack> have" /tmp/git/packet.trace
)

I don't understand this limitation. With the SSH version we skip
straight to saying we "want" with just the 1 "have" line of
"e83c5163316f89bfbde7d9ab23ca2e25604af290".

Why aren't we doing the same over http? I don't get how protocol support
is needed, it's us who decide to send over the "have" lines. Some
variant of this does work over "skipping":

(
rm -rf /tmp/git &&
git clone https://github.com/git/git.git /tmp/git &&
cd /tmp/git &&
git remote add gitlab-shell 
https://github.com/cr-marcstevens/sha1collisiondetection &&
GIT_TRACE_PACKET=/tmp/git/packet.trace git -c 
fetch.negotiationAlgorithm=skipping fetch gitlab-shell &&
grep -c "fetch-pack> have" /tmp/git/packet.trace
)

There we send 14002 "have" lines, which seems expected, but then with
the same thing over SSH we don't send any:

(
rm -rf /tmp/git &&
git clone g...@github.com:git/git.git /tmp/git &&
cd /tmp/git &&
git remote add gitlab-shell 
g...@github.com:cr-marcstevens/sha1collisiondetection &&
GIT_TRACE_PACKET=/tmp/git/packet.trace git -c 
fetch.negotiationAlgorithm=skipping fetch gitlab-shell &&
grep -c "fetch-pack> have" /tmp/git/packet.trace
)

So that seems like another bug, and as an aside, a "skipping"
implementation that sends ~1/4 of the commits in the repo seems way less
aggressive than it should be. I was expecting something that would
gradually "ramp up" from the tips. Where say starting at master/next/pu
we present every 100th commit as a "have" until the 1000th 

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Rafael Ascensão]

On Thu, Sep 27, 2018 at 01:16:19PM -0700, Nickolai Belakovski wrote:
>
> Not to hijack my own thread, but FWIW git branch -r shows remote
> branches in red, but old/new status of a remote branch is ambiguous
> (could have new stuff, could be out of date). Also, git branch -vv
> shows remote tracking branches in blue. One could argue it should be
> red since git branch -r is in red.
>

For me remote branches being red means: they're here but you cannot
write to them. They are like 'read-only/disabled' branches. Under this
interpretation red makes sense.


On Thu, Sep 27, 2018 at 9:02 PM Ævar Arnfjörð Bjarmason  
wrote:
>
> E.g. I thought green here made sense because in "diff" we show the
> old/new as red/green, so the branch you're on is "new" in the same
> sense, i.e. it's what your current state is.
>

I still defend using green and dim green for this case. Because all
these worktrees are in a sense active. They're checked out in some
place. It's just the case that the particular one that we are in is
probably more relevant than the others.

--
Cheers
Rafael Ascensão

Re: Null pointer dereference in rerere.c

[Thomas Gummerer]

On 09/27, Ruud van Asseldonk wrote:
> Hi,
> 
> I just ran into a segmentation fault during a rebase with rerere
> enabled. Inspecting the core dump with gdb shows:

Thanks for reporting this bug

> (gdb) bt
> #0  0x55d673375ce0 in do_rerere_one_path (update=0x7fff03c37f30,
> rr_item=0x55d6746d0b30) at rerere.c:755
> #1  do_plain_rerere (fd=3, rr=0x7fff03c37ef0) at rerere.c:853
> #2  rerere (flags=flags@entry=0) at rerere.c:918
> #3  0x55d673246b01 in am_resolve (state=0x7fff03c38120) at 
> builtin/am.c:1901
> #4  cmd_am (argc=, argv=,
> prefix=) at builtin/am.c:2394
> #5  0x55d67323f975 in run_builtin (argv=,
> argc=, p=) at git.c:346
> #6  handle_builtin (argc=, argv=) at git.c:554
> #7  0x55d6732405e5 in run_argv (argv=0x7fff03c394a0,
> argcp=0x7fff03c394ac) at git.c:606
> #8  cmd_main (argc=, argv=) at git.c:683
> #9  0x55d67323f64a in main (argc=4, argv=0x7fff03c396f8) at 
> common-main.c:43
> (gdb) info locals
> path = 0x55d6746d08e0 ""
> id = 0x55d6746d01e0
> rr_dir = 0x55d6746ccb80
> variant = 
> path = 
> id = 
> rr_dir = 
> variant = 
> both = 
> vid = 
> path = 
> (gdb) print id
> $1 = (struct rerere_id *) 0x55d6746d01e0
> (gdb) print id->collection
> $2 = (struct rerere_dir *) 0x55d6746ccb80
> (gdb) print id->collection->status
> $3 = (unsigned char *) 0x0
> 
> This is using Git 2.17.1 from the 1:2.17.1-1ubuntu0.1 Ubuntu package.
> Looking at the diff between v2.17.1 and master for rerere.c it looks
> like the part of the rerere.c where the null pointer dereference
> happens has not been touched, so the issue might still be there.
> Unfortunately I was unable to reproduce the bug; after removing
> .git/MERGE_RR.lock and restarting the rebase, it completed fine.

I do believe this bug may actually be fixed in master, by 93406a282f
("rerere: fix crash with files rerere can't handle", 2018-08-05).  Do
you by any chance remember if you committed a file that contained
conflict markers during the rebase at some point?

The problem I found at the time looked the same as your backtrace
above in any case.

Would have been nice if you were able to reproduce it, just to make
sure it's not something else we're seeing here.

> Please let me know if there is anything I can do to help diagnose the
> problem, or whether I should report the bug to Ubuntu instead.
> 
> Kind regards,
> 
> Ruud van Asseldonk

Re: [PATCH 3/3] archive: allow archive over HTTP(S) with proto v2

[Josh Steadmon]

On 2018.09.13 09:47, Junio C Hamano wrote:
> Josh Steadmon  writes:
> 
> > Signed-off-by: Josh Steadmon 
> > ---
> >  builtin/archive.c  |  8 +++-
> >  http-backend.c | 10 +-
> >  transport-helper.c |  5 +++--
> >  3 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/builtin/archive.c b/builtin/archive.c
> > index 73831887d..5fa75b3f7 100644
> > --- a/builtin/archive.c
> > +++ b/builtin/archive.c
> > @@ -87,7 +87,13 @@ static int run_remote_archiver(int argc, const char 
> > **argv,
> > status = packet_reader_read();
> > if (status != PACKET_READ_FLUSH)
> > die(_("git archive: expected a flush"));
> > -   }
> > +   } else if (version == protocol_v2 &&
> > +  starts_with(transport->url, "http"))
> > +   /*
> > +* Commands over HTTP require two requests, so there's an
> > +* additional server response to parse.
> > +*/
> > +   discover_version();
> 
> What should happen if the version discovered here is different from
> v2 or the capabilities offered are different from what we saw
> before?  Perhaps we need some sanity checks, as we on this side of
> the connection know we are making two requests, and may even end up
> talking with an instance of "upload-archive" that is different from
> the one we talked with earlier.
> 
> > diff --git a/http-backend.c b/http-backend.c
> > index 458642ef7..d62d583c7 100644
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -32,6 +32,7 @@ struct rpc_service {
> >  static struct rpc_service rpc_service[] = {
> > { "upload-pack", "uploadpack", 1, 1 },
> > { "receive-pack", "receivepack", 0, -1 },
> > +   { "upload-archive", "uploadarchive", 1, 1 },
> >  };
> >  
> >  static struct string_list *get_parameters(void)
> > @@ -637,6 +638,12 @@ static void service_rpc(struct strbuf *hdr, char 
> > *service_name)
> > struct rpc_service *svc = select_service(hdr, service_name);
> > struct strbuf buf = STRBUF_INIT;
> >  
> > +   if (!strcmp(service_name, "git-upload-archive")) {
> > +   /* git-upload-archive doesn't need --stateless-rpc */
> > +   argv[1] = ".";
> > +   argv[2] = NULL;
> > +   }
> > +
> > strbuf_reset();
> > strbuf_addf(, "application/x-git-%s-request", svc->name);
> > check_content_type(hdr, buf.buf);
> > @@ -713,7 +720,8 @@ static struct service_cmd {
> > {"GET", "/objects/pack/pack-[0-9a-f]{40}\\.idx$", get_idx_file},
> >  
> > {"POST", "/git-upload-pack$", service_rpc},
> > -   {"POST", "/git-receive-pack$", service_rpc}
> > +   {"POST", "/git-receive-pack$", service_rpc},
> > +   {"POST", "/git-upload-archive$", service_rpc},
> >  };
> >  
> >  static int bad_request(struct strbuf *hdr, const struct service_cmd *c)
> > diff --git a/transport-helper.c b/transport-helper.c
> > index 143ca008c..b4b96fc89 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -605,7 +605,8 @@ static int process_connect_service(struct transport 
> > *transport,
> > ret = run_connect(transport, );
> > } else if (data->stateless_connect &&
> >(get_protocol_version_config() == protocol_v2) &&
> > -  !strcmp("git-upload-pack", name)) {
> > +  (!strcmp("git-upload-pack", name) ||
> > +   !strcmp("git-upload-archive", name))) {
> > strbuf_addf(, "stateless-connect %s\n", name);
> > ret = run_connect(transport, );
> > if (ret)
> > @@ -639,7 +640,7 @@ static int connect_helper(struct transport *transport, 
> > const char *name,
> >  
> > /* Get_helper so connect is inited. */
> > get_helper(transport);
> > -   if (!data->connect)
> > +   if (!data->connect && !data->stateless_connect)
> > die(_("operation not supported by protocol"));
> 
> This is somewhat curious.  The upload-pack going over HTTP also is
> triggered by the same "stateless-connect" remote helper command, as
> we just saw in the previous hunk, and that support is not new.  Why
> do we need this change then?  What's different between the "data"
> that is obtained by get_helper(transport) for driving upload-pack
> and upload-archive?  Presumably upload-pack was working without this
> change because it also sets the connect bit on, and upload-archive
> does not work without this change because it does not?  Why do these
> two behave differently?

The data struct is not different between upload-pack vs. upload-archive.
Neither of them set the connect bit. The difference is that upload-pack
doesn't need to call transport_connect(), so it never hits
connect_helper().

[PATCH] git-completion.bash: Add completion for stash list

[Steve]

Since stash list accepts git-log options, add the following useful
options that make sense in the context of the `git stash list` command:

  --name-status --oneline --patch-with-stat

Signed-off-by: Steven Fernandez 
---

This is my first patch to the project so please be excuse any process
errors.
Although, I've tried my best to follow the guidelines in
Documentation/SubmittingPatches but I'm not sure if I missed anything.

 contrib/completion/git-completion.bash | 3 +++
 1 file changed, 3 insertions(+)

diff --git contrib/completion/git-completion.bash
contrib/completion/git-completion.bash
index d63d2dffd..06ec6ca11 100644
--- contrib/completion/git-completion.bash
+++ contrib/completion/git-completion.bash
@@ -2567,6 +2567,9 @@ _git_stash ()
drop,--*)
__gitcomp "--quiet"
;;
+   list,--*)
+   __gitcomp "--name-status --oneline --patch-
with-stat"
+   ;;
show,--*|branch,--*)
;;
branch,*)
-- 
2.17.1

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Nickolai Belakovski]

Not to hijack my own thread, but FWIW git branch -r shows remote
branches in red, but old/new status of a remote branch is ambiguous
(could have new stuff, could be out of date). Also, git branch -vv
shows remote tracking branches in blue. One could argue it should be
red since git branch -r is in red.

But yea, probably best to take this topic to its own thread.
On Thu, Sep 27, 2018 at 1:02 PM Ævar Arnfjörð Bjarmason
 wrote:
>
>
> On Thu, Sep 27 2018, Rafael Ascensão wrote:
>
> > On Thu, Sep 27, 2018 at 02:17:08PM -0400, Jeff King wrote:
> >> Do we want to limit this to git-branch, though? Ideally any output you
> >> get from git-branch could be replicated with for-each-ref (or with
> >> a custom "branch --format").
> >>
> >> I.e., could we have a format in ref-filter that matches HEAD, but
> >> returns a distinct symbol for a worktree HEAD? That would allow a few
> >> things:
> >
> > I was going to suggest using dim green and green for elsewhere and here
> > respectively, in a similar way how range-diff uses it to show different
> > versions of the same diff.
>
> It would be really useful to (just via E-Mail to start) itemize the
> colors we use in various places and what they mean.
>
> E.g. I thought green here made sense because in "diff" we show the
> old/new as red/green, so the branch you're on is "new" in the same
> sense, i.e. it's what your current state is.
>
> But maybe there's cases where that doesn't "rhyme" as it were.

[PATCH] git-completion.bash: Add completion for stash list

[Steve]

Since stash list accepts git-log options, add the following useful
options that make sense in the context of the `git stash list` command:

  --name-status --oneline --patch-with-stat

Signed-off-by: Steven Fernandez 
---

This is my first patch to the project so please be excuse any process errors. I
think my mail client messed up the line wrapping in my last mail. Hopefully this
one's better. Although, I've tried my best to follow the guidelines in
Documentation/SubmittingPatches but I'm not sure if I missed anything.

 contrib/completion/git-completion.bash | 3 +++
 1 file changed, 3 insertions(+)

diff --git contrib/completion/git-completion.bash 
contrib/completion/git-completion.bash
index d63d2dffd..06ec6ca11 100644
--- contrib/completion/git-completion.bash
+++ contrib/completion/git-completion.bash
@@ -2567,6 +2567,9 @@ _git_stash ()
drop,--*)
__gitcomp "--quiet"
;;
+   list,--*)
+   __gitcomp "--name-status --oneline --patch-with-stat"
+   ;;
show,--*|branch,--*)
;;
branch,*)
-- 
2.17.1

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 27 2018, Rafael Ascensão wrote:

> On Thu, Sep 27, 2018 at 02:17:08PM -0400, Jeff King wrote:
>> Do we want to limit this to git-branch, though? Ideally any output you
>> get from git-branch could be replicated with for-each-ref (or with
>> a custom "branch --format").
>>
>> I.e., could we have a format in ref-filter that matches HEAD, but
>> returns a distinct symbol for a worktree HEAD? That would allow a few
>> things:
>
> I was going to suggest using dim green and green for elsewhere and here
> respectively, in a similar way how range-diff uses it to show different
> versions of the same diff.

It would be really useful to (just via E-Mail to start) itemize the
colors we use in various places and what they mean.

E.g. I thought green here made sense because in "diff" we show the
old/new as red/green, so the branch you're on is "new" in the same
sense, i.e. it's what your current state is.

But maybe there's cases where that doesn't "rhyme" as it were.

Re: [PATCH] submodule: Alllow staged changes for get_superproject_working_tree

[Stefan Beller]

On Thu, Sep 27, 2018 at 11:12 AM Sam McKelvie  wrote:
>
> Invoking 'git rev-parse --show-superproject-working-tree' exits with
>
> "fatal: BUG: returned path string doesn't match cwd?"
>
> when the superproject has an unmerged entry for the current submodule,
> instead of displaying the superproject's working tree.
>
> The problem is due to the fact that when a merge of the submodule reference
> is in progress, "git ls-files --stage —full-name ”
> returns three seperate entries for the submodule (one for each stage) rather
> than a single entry; e.g.,
>
> $ git ls-files --stage --full-name submodule-child-test
> 16 dbbd2766fa330fa741ea59bb38689fcc2d283ac5 1   submodule-child-test
> 16 f174d1dbfe863a59692c3bdae730a36f2a788c51 2   submodule-child-test
> 16 e6178f3a58b958543952e12824aa2106d560f21d 3   submodule-child-test
>
> The code in get_superproject_working_tree() expected exactly one entry to
> be returned; this patch makes it use the first entry if multiple entries
> are returned.
>
> Test t1500-rev-parse is extended to cover this case.
>
> Signed-off-by: Sam McKelvie 

Thanks for following up, this patch is
Reviewed-by: Stefan Beller 

Thanks for adding the test as well!
Stefan

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Jeff King]

On Thu, Sep 27, 2018 at 03:35:59PM -0400, Jeff King wrote:

> now, we could do:
> 
>   %(if)%(HEAD)%(then)* %(color:bold green)
>   %(else)%(if)%(worktree)%(then)+ %(color:green)
>   %(else)  %(end)%(end)
> 
> (respecting the user's color config, of course, rather than hard-coded
> colors).
> 
> Trying that out, though, I'm not sure if we properly support nested
> if's. That might be a bug we have to fix first.

Sorry, false alarm. I just had a typo in my format.

This seems to work with the patch I posted earlier:

  git for-each-ref \
--format='%(if)%(HEAD)%(then)* %(color:bold 
green)%(else)%(if)%(worktree)%(then)+ %(color:green)%(else) 
%(end)%(end)%(refname)' \
  refs/heads

It sure would be nice if there was a way to insert line breaks without
impacting the output. ;)

-Peff

Re: [PATCH 0/2] negotiator: improve recent behavior + docs

[Jonathan Tan]

> > If you wanted to do this, it seems better to me to just declare a "null"
> > negotiation algorithm that does not perform any negotiation at all.
> 
> I think such an algorithm is a good idea in general, especially for
> testing, and yeah, maybe that's the best way out of this, i.e. to do:
> 
> if git rev-parse {}/HEAD 2>/dev/null
> then
> git fetch --negotiation-tip={}/HEAD {}
> else
> git -c fetch.negotiationAlgorithm=null fetch {}
> fi
> 
> Would such an algorithm be added by overriding default.c's add_tip
> function to never add anything by calling default_negotiator_init()
> followed by null_negotiator_init(), which would only override add_tip?
> (yay C OO)
> 
> If so from fetch-pack.c it looks like there may be the limitation on the
> interface that the negotiator can't exit early (in
> fetch-pack.c:mark_tips). But I've just skimmed this, so maybe I've
> missed something.

(I was reminded to reply to this offlist - sorry for the late reply.)

I think too many things need to be replaced (known_common, add_tip, and
ack all need to do nothing), so it's best to start from scratch. That
way, we also don't need to deal with the subtleties of C OO :-)

> Also, looks like because of the current interface =null and
> --negotiation-tip=* would (somewhat confusingly) do a "real" negotiation
> if done that way, since it'll bypass the API and insert tips for it to
> negotiate, but it looks like overriding next() will get around that.

If you do it as I suggest (in particular, add_tip doing nothing) then
there is the opposite problem that it won't be easy to inform the user
that --negotiation-tip does nothing in this case. Maybe there needs to
be an "accepts_tips" field in struct fetch_negotiator that, if false,
means that custom tips (or any tips) are not accepted, allowing the
caller of the negotiator to print a warning message in this case.

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Jeff King]

On Thu, Sep 27, 2018 at 08:28:04PM +0100, Rafael Ascensão wrote:

> On Thu, Sep 27, 2018 at 02:17:08PM -0400, Jeff King wrote:
> > Do we want to limit this to git-branch, though? Ideally any output you
> > get from git-branch could be replicated with for-each-ref (or with
> > a custom "branch --format").
> > 
> > I.e., could we have a format in ref-filter that matches HEAD, but
> > returns a distinct symbol for a worktree HEAD? That would allow a few
> > things:
> 
> I was going to suggest using dim green and green for elsewhere and here
> respectively, in a similar way how range-diff uses it to show different
> versions of the same diff.

Yeah, I think that's reasonable, and would be enabled by the %(worktree)
placeholder I showed. Just like we do something like:

  %(if)%(HEAD)%(then)* %(color:green)%(else)  %(end)

now, we could do:

  %(if)%(HEAD)%(then)* %(color:bold green)
  %(else)%(if)%(worktree)%(then)+ %(color:green)
  %(else)  %(end)%(end)

(respecting the user's color config, of course, rather than hard-coded
colors).

Trying that out, though, I'm not sure if we properly support nested
if's. That might be a bug we have to fix first.

> But if we're open to change how branches are displayed maybe a config
> option like branch.format (probably not the best name choice) that can
> be set to the 'for-each-ref --format' syntax would be way more flexible.

We have that already, don't we?

> I think the different symbol and dimmed color would be a nice addition,
> but I am leaning towards giving the user the ultimate choice on how they
> want to format their output. (Maybe with dimmed plus symbol as default).

Definitely.

-Peff

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Rafael Ascensão]

On Thu, Sep 27, 2018 at 02:17:08PM -0400, Jeff King wrote:
> Do we want to limit this to git-branch, though? Ideally any output you
> get from git-branch could be replicated with for-each-ref (or with
> a custom "branch --format").
> 
> I.e., could we have a format in ref-filter that matches HEAD, but
> returns a distinct symbol for a worktree HEAD? That would allow a few
> things:

I was going to suggest using dim green and green for elsewhere and here
respectively, in a similar way how range-diff uses it to show different
versions of the same diff.

But if we're open to change how branches are displayed maybe a config
option like branch.format (probably not the best name choice) that can
be set to the 'for-each-ref --format' syntax would be way more flexible.

e.g.
branch.format='%(if:equals=+)...'

I think the different symbol and dimmed color would be a nice addition,
but I am leaning towards giving the user the ultimate choice on how they
want to format their output. (Maybe with dimmed plus symbol as default).

--
Cheers,
Rafael Ascensão

[RFC PATCH v2 4/4] fetch: do not list refs if fetching only hashes

[Jonathan Tan]

If only hash literals are given on a "git fetch" command-line, tag
following is not requested, and the fetch is done using protocol v2, a
list of refs is not required from the remote. Therefore, optimize by
invoking transport_get_remote_refs() only if we need the refs.

Signed-off-by: Jonathan Tan 
---
 builtin/fetch.c | 32 ++--
 t/t5551-http-fetch-smart.sh | 15 +++
 t/t5702-protocol-v2.sh  | 13 +
 3 files changed, 54 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0696abfc2a..4c4f8fa194 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1175,6 +1175,7 @@ static int do_fetch(struct transport *transport,
int retcode = 0;
const struct ref *remote_refs;
struct argv_array ref_prefixes = ARGV_ARRAY_INIT;
+   int must_list_refs = 1;
 
if (tags == TAGS_DEFAULT) {
if (transport->remote->fetch_tags == 2)
@@ -1190,17 +1191,36 @@ static int do_fetch(struct transport *transport,
goto cleanup;
}
 
-   if (rs->nr)
+   if (rs->nr) {
+   int i;
+
refspec_ref_prefixes(rs, _prefixes);
-   else if (transport->remote && transport->remote->fetch.nr)
+
+   /*
+* We can avoid listing refs if all of them are exact
+* OIDs
+*/
+   must_list_refs = 0;
+   for (i = 0; i < rs->nr; i++) {
+   if (!rs->items[i].exact_sha1) {
+   must_list_refs = 1;
+   break;
+   }
+   }
+   } else if (transport->remote && transport->remote->fetch.nr)
refspec_ref_prefixes(>remote->fetch, _prefixes);
 
-   if (ref_prefixes.argc &&
-   (tags == TAGS_SET || (tags == TAGS_DEFAULT))) {
-   argv_array_push(_prefixes, "refs/tags/");
+   if (tags == TAGS_SET || tags == TAGS_DEFAULT) {
+   must_list_refs = 1;
+   if (ref_prefixes.argc)
+   argv_array_push(_prefixes, "refs/tags/");
}
 
-   remote_refs = transport_get_remote_refs(transport, _prefixes);
+   if (must_list_refs)
+   remote_refs = transport_get_remote_refs(transport, 
_prefixes);
+   else
+   remote_refs = NULL;
+
argv_array_clear(_prefixes);
 
ref_map = get_ref_map(transport->remote, remote_refs, rs,
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 771f36f9ff..12b339d239 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -381,6 +381,21 @@ test_expect_success 'using fetch command in remote-curl 
updates refs' '
test_cmp expect actual
 '
 
+test_expect_success 'fetch by SHA-1 without tag following' '
+   SERVER="$HTTPD_DOCUMENT_ROOT_PATH/server" &&
+   rm -rf "$SERVER" client &&
+
+   git init "$SERVER" &&
+   test_commit -C "$SERVER" foo &&
+
+   git clone $HTTPD_URL/smart/server client &&
+
+   test_commit -C "$SERVER" bar &&
+   git -C "$SERVER" rev-parse bar >bar_hash &&
+   git -C client -c protocol.version=0 fetch \
+   --no-tags origin $(cat bar_hash)
+'
+
 test_expect_success 'GIT_REDACT_COOKIES redacts cookies' '
rm -rf clone &&
echo "Set-Cookie: Foo=1" >cookies &&
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index a316bb9bf4..1a97331648 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -79,6 +79,19 @@ test_expect_success 'fetch with git:// using protocol v2' '
grep "fetch< version 2" log
 '
 
+test_expect_success 'fetch by hash without tag following with protocol v2 does 
not list refs' '
+   test_when_finished "rm -f log" &&
+
+   test_commit -C "$daemon_parent" two_a &&
+   git -C "$daemon_parent" rev-parse two_a >two_a_hash &&
+
+   GIT_TRACE_PACKET="$(pwd)/log" git -C daemon_child -c protocol.version=2 
\
+   fetch --no-tags origin $(cat two_a_hash) &&
+
+   grep "fetch< version 2" log &&
+   ! grep "fetch> command=ls-refs" log
+'
+
 test_expect_success 'pull with git:// using protocol v2' '
test_when_finished "rm -f log" &&
 
-- 
2.19.0.605.g01d371f741-goog

[RFC PATCH v2 3/4] transport: list refs before fetch if necessary

[Jonathan Tan]

The built-in bundle transport and the transport helper interface do not
work when transport_fetch_refs() is called immediately after transport
creation. This will be needed in a subsequent patch, so fix this.

Evidence: fetch_refs_from_bundle() relies on data->header being
initialized in get_refs_from_bundle(), and fetch() in transport-helper.c
relies on either data->fetch or data->import being set by get_helper(),
but neither transport_helper_init() nor fetch() calls get_helper().

Up until the introduction of the partial clone feature, this has not
been a problem, because transport_fetch_refs() is always called after
transport_get_remote_refs(). With the introduction of the partial clone
feature, which involves calling transport_fetch_refs() (to fetch objects
by their OIDs) without transport_get_remote_refs(), this is still not a
problem, but only coincidentally - we do not support partially cloning a
bundle, and as for cloning using a transport-helper-using protocol, it
so happens that before transport_fetch_refs() is called, fetch_refs() in
fetch-object.c calls transport_set_option(), which means that the
aforementioned get_helper() is invoked through set_helper_option() in
transport-helper.c.

This could be fixed by fixing the transports themselves, but it doesn't
seem like a good idea to me to open up previously untested code paths;
also, there may be transport helpers in the wild that assume that "list"
is always called before "fetch". Instead, fix this by having
transport_fetch_refs() call transport_get_remote_refs() to ensure that
the latter is always called at least once, unless the transport
explicitly states that it supports fetching without listing refs.

Signed-off-by: Jonathan Tan 
---
 transport-helper.c   |  1 +
 transport-internal.h |  6 ++
 transport.c  | 12 
 3 files changed, 19 insertions(+)

diff --git a/transport-helper.c b/transport-helper.c
index 143ca008c8..7213fa0d32 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1105,6 +1105,7 @@ static struct ref *get_refs_list(struct transport 
*transport, int for_push,
 }
 
 static struct transport_vtable vtable = {
+   0,
set_helper_option,
get_refs_list,
fetch,
diff --git a/transport-internal.h b/transport-internal.h
index 1cde6258a7..004bee5e36 100644
--- a/transport-internal.h
+++ b/transport-internal.h
@@ -6,6 +6,12 @@ struct transport;
 struct argv_array;
 
 struct transport_vtable {
+   /**
+* This transport supports the fetch() function being called
+* without get_refs_list() first being called.
+*/
+   unsigned fetch_without_list : 1;
+
/**
 * Returns 0 if successful, positive if the option is not
 * recognized or is inapplicable, and negative if the option
diff --git a/transport.c b/transport.c
index 4329cca8e5..ea72fff6a6 100644
--- a/transport.c
+++ b/transport.c
@@ -733,6 +733,7 @@ static int disconnect_git(struct transport *transport)
 }
 
 static struct transport_vtable taken_over_vtable = {
+   1,
NULL,
get_refs_via_connect,
fetch_refs_via_pack,
@@ -882,6 +883,7 @@ void transport_check_allowed(const char *type)
 }
 
 static struct transport_vtable bundle_vtable = {
+   0,
NULL,
get_refs_from_bundle,
fetch_refs_from_bundle,
@@ -891,6 +893,7 @@ static struct transport_vtable bundle_vtable = {
 };
 
 static struct transport_vtable builtin_smart_vtable = {
+   1,
NULL,
get_refs_via_connect,
fetch_refs_via_pack,
@@ -1254,6 +1257,15 @@ int transport_fetch_refs(struct transport *transport, 
struct ref *refs)
struct ref **heads = NULL;
struct ref *rm;
 
+   if (!transport->vtable->fetch_without_list)
+   /*
+* Some transports (e.g. the built-in bundle transport and the
+* transport helper interface) do not work when fetching is
+* done immediately after transport creation. List the remote
+* refs anyway (if not already listed) as a workaround.
+*/
+   transport_get_remote_refs(transport, NULL);
+
for (rm = refs; rm; rm = rm->next) {
nr_refs++;
if (rm->peer_ref &&
-- 
2.19.0.605.g01d371f741-goog

[RFC PATCH v2 2/4] transport: do not list refs if possible

[Jonathan Tan]

When all refs to be fetched are exact OIDs, it is possible to perform a
fetch without requiring the remote to list refs if protocol v2 is used.
Teach Git to do this.

This currently has an effect only for lazy fetches done from partial
clones. The change necessary to likewise optimize "git fetch 
" will be done in a subsequent patch.

Signed-off-by: Jonathan Tan 
---
 fetch-pack.c   |  2 +-
 t/t5702-protocol-v2.sh |  5 +
 transport.c| 13 +++--
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 75047a4b2a..15652b4776 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1598,7 +1598,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,
if (nr_sought)
nr_sought = remove_duplicates_in_refs(sought, nr_sought);
 
-   if (!ref) {
+   if (version != protocol_v2 && !ref) {
packet_flush(fd[1]);
die(_("no matching remote head"));
}
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 3beeed4546..a316bb9bf4 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -286,6 +286,11 @@ test_expect_success 'dynamically fetch missing object' '
grep "version 2" trace
 '
 
+test_expect_success 'when dynamically fetching missing object, do not list 
refs' '
+   cat trace &&
+   ! grep "git> command=ls-refs" trace
+'
+
 test_expect_success 'partial fetch' '
rm -rf client "$(pwd)/trace" &&
git init client &&
diff --git a/transport.c b/transport.c
index 5fb9ff6b56..4329cca8e5 100644
--- a/transport.c
+++ b/transport.c
@@ -341,8 +341,17 @@ static int fetch_refs_via_pack(struct transport *transport,
args.server_options = transport->server_options;
args.negotiation_tips = data->options.negotiation_tips;
 
-   if (!data->got_remote_heads)
-   refs_tmp = get_refs_via_connect(transport, 0, NULL);
+   if (!data->got_remote_heads) {
+   int i;
+   int must_list_refs = 0;
+   for (i = 0; i < nr_heads; i++) {
+   if (!to_fetch[i]->exact_oid) {
+   must_list_refs = 1;
+   break;
+   }
+   }
+   refs_tmp = handshake(transport, 0, NULL, must_list_refs);
+   }
 
switch (data->version) {
case protocol_v2:
-- 
2.19.0.605.g01d371f741-goog

[RFC PATCH v2 1/4] transport: allow skipping of ref listing

[Jonathan Tan]

The get_refs_via_connect() function both performs the handshake
(including determining the protocol version) and obtaining the list of
remote refs. However, the fetch protocol v2 supports fetching objects
without the listing of refs, so make it possible for the user to skip
the listing by creating a new handshake() function. This will be used in
a subsequent commit.
---
 transport.c | 29 +
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/transport.c b/transport.c
index 1c76d64aba..5fb9ff6b56 100644
--- a/transport.c
+++ b/transport.c
@@ -252,8 +252,18 @@ static int connect_setup(struct transport *transport, int 
for_push)
return 0;
 }
 
-static struct ref *get_refs_via_connect(struct transport *transport, int 
for_push,
-   const struct argv_array *ref_prefixes)
+/*
+ * Obtains the protocol version from the transport and writes it to
+ * transport->data->version, first connecting if not already connected.
+ *
+ * If the protocol version is one that allows skipping the listing of remote
+ * refs, and must_list_refs is 0, the listing of remote refs is skipped and
+ * this function returns NULL. Otherwise, this function returns the list of
+ * remote refs.
+ */
+static struct ref *handshake(struct transport *transport, int for_push,
+const struct argv_array *ref_prefixes,
+int must_list_refs)
 {
struct git_transport_data *data = transport->data;
struct ref *refs = NULL;
@@ -268,8 +278,10 @@ static struct ref *get_refs_via_connect(struct transport 
*transport, int for_pus
data->version = discover_version();
switch (data->version) {
case protocol_v2:
-   get_remote_refs(data->fd[1], , , for_push,
-   ref_prefixes, transport->server_options);
+   if (must_list_refs)
+   get_remote_refs(data->fd[1], , , for_push,
+   ref_prefixes,
+   transport->server_options);
break;
case protocol_v1:
case protocol_v0:
@@ -283,9 +295,18 @@ static struct ref *get_refs_via_connect(struct transport 
*transport, int for_pus
}
data->got_remote_heads = 1;
 
+   if (reader.line_peeked)
+   BUG("buffer must be empty at the end of handshake()");
+
return refs;
 }
 
+static struct ref *get_refs_via_connect(struct transport *transport, int 
for_push,
+   const struct argv_array *ref_prefixes)
+{
+   return handshake(transport, for_push, ref_prefixes, 1);
+}
+
 static int fetch_refs_via_pack(struct transport *transport,
   int nr_heads, struct ref **to_fetch)
 {
-- 
2.19.0.605.g01d371f741-goog

[RFC PATCH v2 0/4] Avoid ls-refs when possible in protocol v2

[Jonathan Tan]

To answer Junio's questions in [1], I think it's best to include the
full patch set that I'm developing, so here it is. The original patch is
now patch 3 of this set.

[1] https://public-inbox.org/git/xmqq8t3pnphe@gitster-ct.c.googlers.com/

Rearranging Junio's questions:

> ... ah, do you mean that this is not a new feature, but is a bugfix
> for some callers that are not calling get-remote-refs before calling
> fetch-refs, and the bit is to work around the fact that some
> transport not just can function without get-remote-refs first but do
> not want to call it?

Yes, it is the bugfix you describe, except that the bug coincidentally
does not cause any bad behavior. fetch-object.c indeed does not call
get-remote-refs before fetch-refs, but it calls transport_set_option(),
which so happens to do what we need (call set_helper_option()).

However, we need it now, because ...

> But this I do not quite understand.  It looks saying "when asked to
> fetch, if the transport does not allow us to do so without first
> getting the advertisement, lazily do that", and that may be a good
> thing to do, but then aren't the current set of callers already
> calling transport-get-remote-refs elsewhere before they call
> transport-fetch-refs?  IOW, I would have expected to see a matching
> removal, or at least a code that turns an unconditional call to
> get-remote-refs to a conditional one that is done only for the
> transport that lacks the capability, or something along that line.

... this "matching removal" you are talking about is in the subsequent
patch 4. And there is no transport_set_option() to save us this time, so
we really do need this bugfix.

> IOW, I am a bit confused by this comment (copied from an earlier part)
> 
> > +   /**
> > +* This transport supports the fetch() function being called
> > +* without get_refs_list() first being called.
> > +*/
> 
> Shouldn't it read more like "this transport does not want its
> get-refs-list called when fetch-refs is done"?
> 
> I dunno.

I'm not sure I understand - transports generally don't care if
get-refs-list is called after fetch-refs. Also, this already happens
when fetching with tag following from a server that does not support tag
following, using a transport that supports reuse.

Jonathan Tan (4):
  transport: allow skipping of ref listing
  transport: do not list refs if possible
  transport: list refs before fetch if necessary
  fetch: do not list refs if fetching only hashes

 builtin/fetch.c | 32 +-
 fetch-pack.c|  2 +-
 t/t5551-http-fetch-smart.sh | 15 +++
 t/t5702-protocol-v2.sh  | 18 +
 transport-helper.c  |  1 +
 transport-internal.h|  6 +
 transport.c | 54 -
 7 files changed, 115 insertions(+), 13 deletions(-)

-- 
2.19.0.605.g01d371f741-goog

Re: [PATCH v2 0/4] git-commit-graph.txt: various cleanups

[Martin Ågren]

Hi Derrick

On Thu, 27 Sep 2018 at 21:16, Derrick Stolee  wrote:
> Thanks! This version satisfies my concerns and looks good to me.
>
> Reviewed-by: Derrick Stolee 

Thanks for the spectacularly snappy review. I don't expect commit graphs
to help my use cases a lot, but I still wanted to try them out a little
and stumbled on the `*` lists. Thanks for doing this work!

Martin

Re: [PATCH v2 0/4] git-commit-graph.txt: various cleanups

[Derrick Stolee]

On 9/27/2018 3:12 PM, Martin Ågren wrote:

This v2 starts with the same two patches as v1 did, then goes on to
change "[commit] graph file" to "commit-graph file" with a dash, to
match other instances as well as Derrick's feedback.

Thanks! This version satisfies my concerns and looks good to me.

Reviewed-by: Derrick Stolee 

[PATCH v2 1/4] git-commit-graph.txt: fix bullet lists

[Martin Ågren]

We have a couple of bullet items which span multiple lines, and where we
have prefixed each line with a `*`. (This might be the result of a text
editor trying to help.) This results in each line being typeset as a
separate bullet item. Drop the extra `*`.

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index dececb79d7..f42f2a1481 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -73,7 +73,7 @@ $ git commit-graph write
 
 
 * Write a graph file, extending the current graph file using commits
-* in .
+  in .
 +
 
 $ echo  | git commit-graph write --stdin-packs
@@ -86,7 +86,7 @@ $ git show-ref -s | git commit-graph write --stdin-commits
 
 
 * Write a graph file containing all commits in the current
-* commit-graph file along with those reachable from HEAD.
+  commit-graph file along with those reachable from HEAD.
 +
 
 $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
-- 
2.19.0.216.g2d3b1c576c

[PATCH v2 4/4] Doc: refer to the "commit-graph file" with dash

[Martin Ågren]

The file processed by `git commit-graph` is referred to as the
"commit-graph file", also with a dash. We have a few references to the
"commit graph file", though, without the dash. These occur in
git-commit-graph.txt as well as in Doc/technical/commit-graph.txt. Fix
them.

Do not change the references to the "commit graph" (without "... file")
as a data structure.

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt   | 12 ++--
 Documentation/technical/commit-graph.txt |  8 
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index f0a171..624470e198 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -3,7 +3,7 @@ git-commit-graph(1)
 
 NAME
 
-git-commit-graph - Write and verify Git commit graph files
+git-commit-graph - Write and verify Git commit-graph files
 
 
 SYNOPSIS
@@ -17,16 +17,16 @@ SYNOPSIS
 DESCRIPTION
 ---
 
-Manage the serialized commit graph file.
+Manage the serialized commit-graph file.
 
 
 OPTIONS
 ---
 --object-dir::
-   Use given directory for the location of packfiles and commit graph
+   Use given directory for the location of packfiles and commit-graph
file. This parameter exists to specify the location of an alternate
that only has the objects directory, not a full `.git` directory. The
-   commit graph file is expected to be at `/info/commit-graph` and
+   commit-graph file is expected to be at `/info/commit-graph` and
the packfiles are expected to be in `/pack`.
 
 
@@ -34,7 +34,7 @@ COMMANDS
 
 'write'::
 
-Write a commit graph file based on the commits found in packfiles.
+Write a commit-graph file based on the commits found in packfiles.
 +
 With the `--stdin-packs` option, generate the new commit graph by
 walking objects only in the specified pack-indexes. (Cannot be combined
@@ -66,7 +66,7 @@ database. Used to check for corrupted data.
 EXAMPLES
 
 
-* Write a commit graph file for the packed commits in your local `.git`
+* Write a commit-graph file for the packed commits in your local `.git`
   directory.
 +
 
diff --git a/Documentation/technical/commit-graph.txt 
b/Documentation/technical/commit-graph.txt
index c664acbd76..6b7dde011e 100644
--- a/Documentation/technical/commit-graph.txt
+++ b/Documentation/technical/commit-graph.txt
@@ -15,13 +15,13 @@ There are two main costs here:
 1. Decompressing and parsing commits.
 2. Walking the entire graph to satisfy topological order constraints.
 
-The commit graph file is a supplemental data structure that accelerates
+The commit-graph file is a supplemental data structure that accelerates
 commit graph walks. If a user downgrades or disables the 'core.commitGraph'
 config setting, then the existing ODB is sufficient. The file is stored
 as "commit-graph" either in the .git/objects/info directory or in the info
 directory of an alternate.
 
-The commit graph file stores the commit graph structure along with some
+The commit-graph file stores the commit graph structure along with some
 extra metadata to speed up graph walks. By listing commit OIDs in lexi-
 cographic order, we can identify an integer position for each commit and
 refer to the parents of a commit using those integer positions. We use
@@ -103,7 +103,7 @@ that of a parent.
 Design Details
 --
 
-- The commit graph file is stored in a file named 'commit-graph' in the
+- The commit-graph file is stored in a file named 'commit-graph' in the
   .git/objects/info directory. This could be stored in the info directory
   of an alternate.
 
@@ -127,7 +127,7 @@ Future Work
 - 'log --topo-order'
 - 'tag --merged'
 
-- A server could provide a commit graph file as part of the network protocol
+- A server could provide a commit-graph file as part of the network protocol
   to avoid extra calculations by clients. This feature is only of benefit if
   the user is willing to trust the file, because verifying the file is correct
   is as hard as computing it from scratch.
-- 
2.19.0.216.g2d3b1c576c

[PATCH v2 2/4] git-commit-graph.txt: typeset more in monospace

[Martin Ågren]

While we're here, fix an instance of "folder" to be "directory".

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index f42f2a1481..6ac610f016 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -25,9 +25,9 @@ OPTIONS
 --object-dir::
Use given directory for the location of packfiles and commit graph
file. This parameter exists to specify the location of an alternate
-   that only has the objects directory, not a full .git directory. The
-   commit graph file is expected to be at /info/commit-graph and
-   the packfiles are expected to be in /pack.
+   that only has the objects directory, not a full `.git` directory. The
+   commit graph file is expected to be at `/info/commit-graph` and
+   the packfiles are expected to be in `/pack`.
 
 
 COMMANDS
@@ -66,14 +66,15 @@ database. Used to check for corrupted data.
 EXAMPLES
 
 
-* Write a commit graph file for the packed commits in your local .git folder.
+* Write a commit graph file for the packed commits in your local `.git`
+  directory.
 +
 
 $ git commit-graph write
 
 
 * Write a graph file, extending the current graph file using commits
-  in .
+  in ``.
 +
 
 $ echo  | git commit-graph write --stdin-packs
@@ -86,7 +87,7 @@ $ git show-ref -s | git commit-graph write --stdin-commits
 
 
 * Write a graph file containing all commits in the current
-  commit-graph file along with those reachable from HEAD.
+  commit-graph file along with those reachable from `HEAD`.
 +
 
 $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
-- 
2.19.0.216.g2d3b1c576c

[PATCH v2 0/4] git-commit-graph.txt: various cleanups

[Martin Ågren]

This v2 starts with the same two patches as v1 did, then goes on to
change "[commit] graph file" to "commit-graph file" with a dash, to
match other instances as well as Derrick's feedback.

Martin Ågren (4):
  git-commit-graph.txt: fix bullet lists
  git-commit-graph.txt: typeset more in monospace
  git-commit-graph.txt: refer to "*commit*-graph file"
  Doc: refer to the "commit-graph file" with dash

 Documentation/git-commit-graph.txt   | 31 
 Documentation/technical/commit-graph.txt |  8 +++---
 2 files changed, 20 insertions(+), 19 deletions(-)

Range-diff against v1:
1:  222721870b = 1:  837ef2f231 git-commit-graph.txt: fix bullet lists
2:  acac5c3584 = 2:  9759a162ca git-commit-graph.txt: typeset more in monospace
3:  65f42c947a ! 3:  759bc886d8 git-commit-graph.txt: refer to "*commit* graph 
file"
@@ -1,17 +1,17 @@
 Author: Martin Ågren 
 
-git-commit-graph.txt: refer to "*commit* graph file"
+git-commit-graph.txt: refer to "*commit*-graph file"
 
-This document sometimes refers to the "commit graph file" as just "the
+This document sometimes refers to the "commit-graph file" as just "the
 graph file". This saves a couple of words here and there at the risk of
 confusion. In particular, the documentation for `git commit-graph read`
 appears to suggest that there are indeed different types of graph 
files.
 
 Let's just write out the full name everywhere.
 
-The full name, by the way, is not the "commit-graph file" with a dash,
-cf. the synopsis. Use the dashless form. (The next commit will fix the
-remaining few instances of the "commit-graph file" in this document.)
+The full name, by the way, is not the dash-less "commit graph file".
+Use the dashed form. (The next commit will fix the remaining few
+instances of the "commit graph file" in this document.)
 
 Signed-off-by: Martin Ågren 
 
@@ -24,7 +24,7 @@
  
 -Read a graph file given by the commit-graph file and output basic
 -details about the graph file. Used for debugging purposes.
-+Read the commit graph file and output basic details about it.
++Read the commit-graph file and output basic details about it.
 +Used for debugging purposes.
  
  'verify'::
@@ -35,7 +35,7 @@
  
 -* Write a graph file, extending the current graph file using commits
 -  in ``.
-+* Write a commit graph file, extending the current commit graph file
++* Write a commit-graph file, extending the current commit-graph file
 +  using commits in ``.
  +
  
@@ -43,16 +43,14 @@
  
  
 -* Write a graph file containing all reachable commits.
-+* Write a commit graph file containing all reachable commits.
++* Write a commit-graph file containing all reachable commits.
  +
  
  $ git show-ref -s | git commit-graph write --stdin-commits
  
  
 -* Write a graph file containing all commits in the current
--  commit-graph file along with those reachable from `HEAD`.
-+* Write a commit graph file containing all commits in the current
-+  commit graph file along with those reachable from `HEAD`.
++* Write a commit-graph file containing all commits in the current
+   commit-graph file along with those reachable from `HEAD`.
  +
  
- $ git rev-parse HEAD | git commit-graph write --stdin-commits --append
4:  fc81147ea4 < -:  -- git-commit-graph.txt: refer to the "commit 
graph file" without dash
-:  -- > 4:  99b64287ec Doc: refer to the "commit-graph file" with dash
-- 
2.19.0.216.g2d3b1c576c

[PATCH v2 3/4] git-commit-graph.txt: refer to "*commit*-graph file"

[Martin Ågren]

This document sometimes refers to the "commit-graph file" as just "the
graph file". This saves a couple of words here and there at the risk of
confusion. In particular, the documentation for `git commit-graph read`
appears to suggest that there are indeed different types of graph files.

Let's just write out the full name everywhere.

The full name, by the way, is not the dash-less "commit graph file".
Use the dashed form. (The next commit will fix the remaining few
instances of the "commit graph file" in this document.)

Signed-off-by: Martin Ågren 
---
 Documentation/git-commit-graph.txt | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-commit-graph.txt 
b/Documentation/git-commit-graph.txt
index 6ac610f016..f0a171 100644
--- a/Documentation/git-commit-graph.txt
+++ b/Documentation/git-commit-graph.txt
@@ -54,8 +54,8 @@ existing commit-graph file.
 
 'read'::
 
-Read a graph file given by the commit-graph file and output basic
-details about the graph file. Used for debugging purposes.
+Read the commit-graph file and output basic details about it.
+Used for debugging purposes.
 
 'verify'::
 
@@ -73,20 +73,20 @@ EXAMPLES
 $ git commit-graph write
 
 
-* Write a graph file, extending the current graph file using commits
-  in ``.
+* Write a commit-graph file, extending the current commit-graph file
+  using commits in ``.
 +
 
 $ echo  | git commit-graph write --stdin-packs
 
 
-* Write a graph file containing all reachable commits.
+* Write a commit-graph file containing all reachable commits.
 +
 
 $ git show-ref -s | git commit-graph write --stdin-commits
 
 
-* Write a graph file containing all commits in the current
+* Write a commit-graph file containing all commits in the current
   commit-graph file along with those reachable from `HEAD`.
 +
 
-- 
2.19.0.216.g2d3b1c576c

Re: [PATCH] branch: colorize branches checked out in a linked working tree the same way as the current branch is colorized

[Jeff King]

On Thu, Sep 27, 2018 at 11:39:26AM -0700, Nickolai Belakovski wrote:

> Thanks for the feedback Peff. I actually agree with all your points.
> I'd considered an approach like what you proposed, but rejected it for
> the first iteration in an effort to keep scope limited and see what
> kind of feedback I'd get overall (like would people even want this?).
> This is a much better approach, and also gives a path for listing the
> worktree path in the verbose output.

Great. If you go that route, feel free to use whatever bits of my patch
are useful. I tested it only by running "for-each-ref" once, so it might
need some more help. Definitely tests and documentation at the least. :)

-Peff

Re: [PATCH] worktree: add per-worktree config files

[Duy Nguyen]

On Thu, Sep 27, 2018 at 8:34 PM Ævar Arnfjörð Bjarmason
 wrote:
> ...
>
> So there is some special casing of .git/config somewhere. I looked into
> this ages ago, and don't remember where that's done.

Thanks! At least know I have some clues to look into (and will do).
-- 
Duy

Re: [PATCH v2 2/4] archive: use packet_reader for communications

[Stefan Beller]

On Wed, Sep 26, 2018 at 6:25 PM Josh Steadmon  wrote:
>
> Using packet_reader will simplify version detection and capability
> handling, which will make implementation of protocol v2 support in
> git-archive easier.
>
> This refactoring does not change the behavior of "git archive".
>
> Signed-off-by: Josh Steadmon 

This patch is
Reviewed-by: Stefan Beller 

Thanks!

> ---
>  builtin/archive.c | 23 ++-
>  1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/archive.c b/builtin/archive.c
> index e74f675390..4eb547c5b7 100644
> --- a/builtin/archive.c
> +++ b/builtin/archive.c
> @@ -27,10 +27,11 @@ static int run_remote_archiver(int argc, const char 
> **argv,
>const char *remote, const char *exec,
>const char *name_hint)
>  {
> -   char *buf;
> int fd[2], i, rv;
> struct transport *transport;
> struct remote *_remote;
> +   struct packet_reader reader;
> +   enum packet_read_status status;
>
> _remote = remote_get(remote);
> if (!_remote->url[0])
> @@ -38,6 +39,8 @@ static int run_remote_archiver(int argc, const char **argv,
> transport = transport_get(_remote, _remote->url[0]);
> transport_connect(transport, "git-upload-archive", exec, fd);
>
> +   packet_reader_init(, fd[0], NULL, 0, 
> PACKET_READ_CHOMP_NEWLINE);
> +
> /*
>  * Inject a fake --format field at the beginning of the
>  * arguments, with the format inferred from our output
> @@ -53,18 +56,20 @@ static int run_remote_archiver(int argc, const char 
> **argv,
> packet_write_fmt(fd[1], "argument %s\n", argv[i]);
> packet_flush(fd[1]);
>
> -   buf = packet_read_line(fd[0], NULL);
> -   if (!buf)
> +   status = packet_reader_read();
> +
> +   if (status != PACKET_READ_NORMAL || reader.pktlen <= 0)
> die(_("git archive: expected ACK/NAK, got a flush packet"));
> -   if (strcmp(buf, "ACK")) {
> -   if (starts_with(buf, "NACK "))
> -   die(_("git archive: NACK %s"), buf + 5);
> -   if (starts_with(buf, "ERR "))
> -   die(_("remote error: %s"), buf + 4);
> +   if (strcmp(reader.line, "ACK")) {
> +   if (starts_with(reader.line, "NACK "))
> +   die(_("git archive: NACK %s"), reader.line + 5);
> +   if (starts_with(reader.line, "ERR "))
> +   die(_("remote error: %s"), reader.line + 4);
> die(_("git archive: protocol error"));
> }
>
> -   if (packet_read_line(fd[0], NULL))
> +   status = packet_reader_read();
> +   if (status == PACKET_READ_NORMAL && reader.pktlen > 0)
> die(_("git archive: expected a flush"));
>
> /* Now, start reading from fd[0] and spit it out to stdout */
> --
> 2.19.0.605.g01d371f741-goog
>