Re: [PATCH 18/41] index-pack: abstract away hash function constant

[Duy Nguyen]

On Fri, Apr 27, 2018 at 11:08 PM, brian m. carlson
 wrote:
> On Thu, Apr 26, 2018 at 05:46:28PM +0200, Duy Nguyen wrote:
>> On Wed, Apr 25, 2018 at 8:49 PM, Martin Ågren  wrote:
>> > Once that is accomplished, I sort of suspect that this code will want to
>> > be updated to not always blindly use the_hash_algo, but to always work
>> > with SHA-1 sizes. Or rather, this would turn into more generic code to
>> > handle both "v2 with SHA-1" and "v3 with some hash function(s)". This
>> > commit might be a good first step in that direction.
>>
>> I also have an uneasy feeling when things this close to on-disk file
>> format get hash-agnostic treatment. I think we would need to start
>> adding new file formats soon, from bottom up with simple things like
>> loose object files (cat-file and hash-object should be enough to test
>> blobs...), then moving up to pack files and more. This is when we can
>> really decide where to use the new hash and whether we should keep
>> some hashes as sha-1.
>
> I agree that this is work which needs to be done soon.  There are
> basically a couple of pieces we need to handle NewHash:
>
> * Remove the dependencies on SHA-1 as much as possible.
> * Get the tests to pass with a different hash (almost done for 160-bit
>   hash; in progress for 256-bit hashes).

This step sounds good on paper but realistically could be a nightmare for you.

I tried to implement a simple cat-file/hash-object combination with my
imaginary newhash, which sounded straightforward to me since you have
done a lot of heavylifting. To my surprise I hit a lot more problems.
My point is, when I concentrate on just a few simple cases like this,
I have a smaller scope to work with and could quickly identify
problems. When you work on the scale of the test suite, it's really
hard to know where the problem is (and you don't even know what areas
are newhash-safe).

Anyway my cat-file/hash-object modification could be found here. I
probably will polish and send out a few good patches from it.

https://github.com/pclouds/git/commits/new-hash
-- 
Duy

Re: [PATCH v6 06/11] Add a test for `git replace --convert-graft-file`

[SZEDER Gábor]

Hallo Johannes,

> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index c630aba657e..bed86a0af3d 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -444,4 +444,24 @@ test_expect_success GPG '--graft on a commit with a 
> mergetag' '

Note the GPG prereq of the previous test.

>   git replace -d $HASH10
>  '
>  
> +test_expect_success '--convert-graft-file' '
> + : add and convert graft file &&
> + printf "%s\n%s %s\n\n# comment\n%s\n" \
> + $(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \

This test relies on the piece of history that was created two tests
earlier in 'set up a merge commit with a mergetag', and that test, too,
has the GPG prereq.  So if there is no GPG installed, then that test
won't be executed, the history needed by this test won't be created,
HEAD won't have a second parent, and I'm sure you know where this is
going:

  ok 32 # skip set up a merge commit with a mergetag (missing GPG)
  <... snip ...>
  ok 33 # skip --graft on a commit with a mergetag (missing GPG)
  <... snip ...>
  ++git log --graph --oneline
  * ffccc9d hello: again 3 more lines
  * 14ac020 hello: 2 more lines
  * 093e41a hello: BUG fixed
  * 40237c8 hello: 1 more line
  * 8fc2a8e hello: 2 more lines
  * 4217adb hello: 4 more lines with a BUG
  * 00ad688 hello: 4 lines
  ++: add and convert graft file
  +++git rev-parse 'HEAD^^' 'HEAD^' 'HEAD^^' 'HEAD^2'
  fatal: ambiguous argument 'HEAD^2': unknown revision or path not in the 
working tree.
  Use '--' to separate paths from revisions, like this:
  'git  [...] -- [...]'
  ++printf '%s\n%s %s\n\n# comment\n%s\n' 
093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c 
14ac020163ea60a9d683ce68e36c946f31ecc856 
093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c 'HEAD^2'
  ++cat .git/info/grafts
  093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c
  14ac020163ea60a9d683ce68e36c946f31ecc856 
093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c
  # comment
  HEAD^2
  ++git replace --convert-graft-file
  hint: Support for /info/grafts is deprecated
  hint: and will be removed in a future Git version.
  hint: 
  hint: Please use "git replace --convert-graft-file"
  hint: to convert the grafts into replace refs.
  hint: 
  hint: Turn this message off by running
  hint: "git config advice.graftFileDeprecated false"
  error: bad graft data: HEAD^2
  error: new commit is the same as the old one: 
'14ac020163ea60a9d683ce68e36c946f31ecc856'
  error: Not a valid object name: 'HEAD^2'
  warning: could not convert the following graft(s):
  14ac020163ea60a9d683ce68e36c946f31ecc856 
093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c
  HEAD^2
  error: last command exited with $?=1
  not ok 34 - --convert-graft-file


> + >.git/info/grafts &&
> + git replace --convert-graft-file &&
> + test_path_is_missing .git/info/grafts &&
> +
> + : verify that the history is now "grafted" &&
> + git rev-list HEAD >out &&
> + test_line_count = 4 out &&
> +
> + : create invalid graft file and verify that it is not deleted &&
> + test_when_finished "rm -f .git/info/grafts" &&
> + echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts &&
> + test_must_fail git replace --convert-graft-file 2>err &&
> + test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" err &&
> + test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts
> +'
> +
>  test_done
> -- 
> 2.17.0.windows.1.33.gfcbb1fa0445

Branch deletion question / possible bug?

[Tang (US), Pik S]

Hi,

I discovered that I was able to delete the feature branch I was in, due to some 
fat fingering on my part and case insensitivity.  I never realized this could 
be done before.  A quick google search did not give me a whole lot to work 
with...  

Steps to reproduce:
1. Create a feature branch, "editCss"
2. git checkout master
3. git checkout editCSS
4. git checkout editCss
5. git branch -d editCSS

Normally, it should have been impossible for a user to delete the branch 
they're on.  And the deletion left me in a weird state that took a while to dig 
out of.

I know this was a user error, but I was also wondering if this was a bug.


Thanks,

Pik Tang

Re: git merge banch w/ different submodule revision

[Elijah Newren]

Hi,

On Fri, Apr 27, 2018 at 3:37 AM, Middelschulte, Leif
 wrote:
> Am Donnerstag, den 26.04.2018, 17:19 -0700 schrieb Elijah Newren:
>> On Thu, Apr 26, 2018 at 3:49 AM, Middelschulte, Leif
>>  wrote:

>> > Problem case: Merge either branch into the other
>> >
>> > Expected behavior: Merge conflict.
>> >
>> > Actual behavior: Auto merge without conflicts.
>> >
>> > Note 1: A merge conflict does occur, if the sourced revisions do *not* 
>> > have a linear history

Let me just note that I don't actually use submodules myself, and
rarely run across them, so as far as users expect submodules should
behave I may have to defer to others.  But it was particularly this
sentence of yours that caught my attention and got me to respond.  I
may have misunderstood which repository had the non-linear history,
but...


>> But, there is some further smarts in that if either A or B point at
>> commits that contain the other in their history and both contain the
>> commit that O points at, then you can just do a fast-forward update to
>> the newest.

This particular paragraph, is relevant to your example; more details below.

>> You didn't tell us how the merge-base (cd5e1a5 from the diagram you
>> gave) differed in your example here between the two repositories.  In
>> fact, the non-linear case could have several merge-bases, in which
>> case they all become potentially relevant (as does their merge-bases
>> since at that point you'll trigger the recursive portion of
>> merge-recursive).  Giving us that info might help us point out what
>> happened, though if either the fast-forward logic comes into play or
>> the recursive logic gets in the mix, then we may need you to provide a
>> testcase (or access to the repo in question) in order to explain it
>> and/or determine if you've found a bug.
>
> I placed two reositories here: 
> https://gitlab.com/foss-contributions/git-examples/network/develop
> The access should be public w/o login.
>
> If you prefer the examples to be placed somewhere else, let me know.

So the only thing I see here is a single repository, which contains a
submodule with linear history.  (unless I was grabbing it wrong; I
just tried `git clone --recurse-submodules
https://gitlab.com/foss-contributions/git-examples`)  Do you also have
an example with non-linear history demonstrating your claim that it
behaves differently, for comparison?


Anyway, in this case you had both branches updating the submodule to
something newer (to a fast-forward update of what it previously was),
but one side advanced it further than the other side did (in
particular, to what turned out to be a fast-forward update of what the
other branch used).  That means the whole fast-forwarding logic of
commit 68d03e4a6e44 ("Implement automatic fast-forward merge for
submodules", 2010-07-07)) came into play.

I would expect that a different example involving non-linear history
would behave the same, if both sides update the submodule in a fashion
that is just fast-forwarding and one commit contains the other in its
history.  I'm curious if you have a counter example.

[PATCH 6/6] rebase --rebase-merges: root commits can be cousins, too

[Johannes Schindelin]

Reported by Wink Saville: when rebasing with no-rebase-cousins, we
will want to refrain from rebasing all of them, even when they are
root commits.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c  |  3 ++-
 t/t3430-rebase-merges.sh | 25 +
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index ad5ff2709a6..2bcd13e1fc6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3883,7 +3883,8 @@ static int make_script_with_merges(struct 
pretty_print_context *pp,
}
 
if (!commit)
-   fprintf(out, "%s onto\n", cmd_reset);
+   fprintf(out, "%s %s\n", cmd_reset,
+   rebase_cousins ? "onto" : "[new root]");
else {
const char *to = NULL;
 
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 5543f1d5a34..ce6de6f491e 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -287,5 +287,30 @@ test_expect_success 'a "merge" into a root commit is a 
fast-forward' '
test_cmp_rev HEAD $head
 '
 
+test_expect_success 'A root commit can be a cousin, treat it that way' '
+   git checkout --orphan khnum &&
+   test_commit yama &&
+   git checkout -b asherah master &&
+   test_commit shamkat &&
+   git merge --allow-unrelated-histories khnum &&
+   test_tick &&
+   git rebase -f -r HEAD^ &&
+   ! test_cmp_rev HEAD^2 khnum &&
+   test_cmp_graph HEAD^.. <<-\EOF &&
+   *   Merge branch '\''khnum'\'' into asherah
+   |\
+   | * yama
+   o shamkat
+   EOF
+   test_tick &&
+   git rebase --rebase-merges=rebase-cousins HEAD^ &&
+   test_cmp_graph HEAD^.. <<-\EOF
+   *   Merge branch '\''khnum'\'' into asherah
+   |\
+   | * yama
+   |/
+   o shamkat
+   EOF
+'
 
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 5/6] rebase --rebase-merges: a "merge" into a new root is a fast-forward

[Johannes Schindelin]

When a user provides a todo list containing something like

reset [new root]
merge my-branch

let's do the same as if pulling into an orphan branch: simply
fast-forward.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c  | 12 
 t/t3430-rebase-merges.sh | 13 +
 2 files changed, 25 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index d10ebd62520..ad5ff2709a6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2850,6 +2850,18 @@ static int do_merge(struct commit *commit, const char 
*arg, int arg_len,
goto leave_merge;
}
 
+   if (opts->have_squash_onto &&
+   !oidcmp(_commit->object.oid, >squash_onto)) {
+   /*
+* When the user tells us to "merge" something into a
+* "[new root]", let's simply fast-forward to the merge head.
+*/
+   rollback_lock_file();
+   ret = fast_forward_to(_commit->object.oid,
+  _commit->object.oid, 0, opts);
+   goto leave_merge;
+   }
+
if (commit) {
const char *message = get_commit_buffer(commit, NULL);
const char *body;
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 35260862fcb..5543f1d5a34 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -275,4 +275,17 @@ test_expect_success 'root commits' '
test_cmp_rev HEAD $before
 '
 
+test_expect_success 'a "merge" into a root commit is a fast-forward' '
+   head=$(git rev-parse HEAD) &&
+   cat >script-from-scratch <<-EOF &&
+   reset [new root]
+   merge $head
+   EOF
+   test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+   test_tick &&
+   git rebase -i -r HEAD^ &&
+   test_cmp_rev HEAD $head
+'
+
+
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 3/6] rebase -i --root: let the sequencer handle even the initial part

[Johannes Schindelin]

In this developer's earlier attempt to accelerate interactive rebases by
converting large parts from Unix shell script into portable, performant
C, the --root handling was specifically excluded (to simplify the task a
little bit; it still took over a year to get that reduced set of patches
into Git proper).

This patch ties up that loose end: now only --preserve-merges uses the
slow Unix shell script implementation to perform the interactive rebase.

As the rebase--helper reports progress to stderr (unlike the scripted
interactive rebase, which reports it to stdout, of all places), we have
to adjust a couple of tests that did not expect that for `git rebase -i
--root`.

This patch fixes -- at long last! -- the really old bug reported in
6a6bc5bdc4d (add tests for rebasing root, 2013-06-06) that rebasing with
--root *always* rewrote the root commit, even if there were no changes.

The bug still persists in --preserve-merges mode, of course, but that
mode will be deprecated as soon as the new --rebase-merges mode
stabilizes, anyway.

Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh|  4 +++-
 t/t3404-rebase-interactive.sh | 19 +--
 t/t3421-rebase-topology-linear.sh |  6 +++---
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index cbf44f86482..2f4941d0fc9 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -894,6 +894,8 @@ init_revisions_and_shortrevisions () {
else
revisions=$onto...$orig_head
shortrevisions=$shorthead
+   test -z "$squash_onto" ||
+   echo "$squash_onto" >"$state_dir"/squash-onto
fi
 }
 
@@ -948,7 +950,7 @@ EOF
die "Could not skip unnecessary pick commands"
 
checkout_onto
-   if test -z "$rebase_root" && test ! -d "$rewritten"
+   if test ! -d "$rewritten"
then
require_clean_work_tree "rebase"
exec git rebase--helper ${force_rebase:+--no-ff} 
$allow_empty_message \
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 59c766540e5..c65826ddace 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1204,10 +1204,6 @@ test_expect_success 'drop' '
test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
 '
 
-cat >expect expect actual.2 &&
+   cr_to_nl actual &&
test_i18ncmp expect actual &&
test D = $(git cat-file commit HEAD | sed -ne \$p)
 '
diff --git a/t/t3421-rebase-topology-linear.sh 
b/t/t3421-rebase-topology-linear.sh
index e7438ad06ac..99b2aac9219 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -328,9 +328,9 @@ test_run_rebase () {
test_cmp_rev c HEAD
"
 }
-test_run_rebase failure ''
-test_run_rebase failure -m
-test_run_rebase failure -i
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
 test_run_rebase failure -p
 
 test_run_rebase () {
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 2/6] sequencer: learn about the special "fake root commit" handling

[Johannes Schindelin]

When an interactive rebase wants to recreate a root commit, it
- first creates a new, empty root commit,
- checks it out,
- converts the next `pick` command so that it amends the empty root
  commit

Introduce support in the sequencer to handle such an empty root commit,
by looking for the file /rebase-merge/squash-onto; if it exists
and contains a commit name, the sequencer will compare the HEAD to said
root commit, and if identical, a new root commit will be created.

While converting scripted code into proper, portable C, we also do away
with the old "amend with an empty commit message, then cherry-pick
without committing, then amend again" dance and replace it with code
that uses the internal API properly to do exactly what we want: create a
new root commit.

To keep the implementation simple, we always spawn `git commit` to create
new root commits.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 104 ++--
 sequencer.h |   4 ++
 2 files changed, 105 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 90c8218aa9a..fc124596b53 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -125,6 +125,12 @@ static GIT_PATH_FUNC(rebase_path_rewritten_list, 
"rebase-merge/rewritten-list")
 static GIT_PATH_FUNC(rebase_path_rewritten_pending,
"rebase-merge/rewritten-pending")
 
+/*
+ * The path of the file containig the OID of the "squash onto" commit, i.e.
+ * the dummy commit used for `reset [new root]`.
+ */
+static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
+
 /*
  * The path of the file listing refs that need to be deleted after the rebase
  * finishes. This is used by the `label` command to record the need for 
cleanup.
@@ -470,7 +476,8 @@ static int fast_forward_to(const struct object_id *to, 
const struct object_id *f
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, "HEAD",
-  to, unborn ? _oid : from,
+  to, unborn && !is_rebase_i(opts) ?
+  _oid : from,
   0, sb.buf, ) ||
ref_transaction_commit(transaction, )) {
ref_transaction_free(transaction);
@@ -692,6 +699,42 @@ static char *get_author(const char *message)
return NULL;
 }
 
+static const char *read_author_ident(struct strbuf *buf)
+{
+   char *p, *p2;
+
+   if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)
+   return NULL;
+
+   for (p = buf->buf; *p; p++)
+   if (skip_prefix(p, "'''", (const char **)))
+   strbuf_splice(buf, p - buf->buf, p2 - p, "'", 1);
+   else if (*p == '\'')
+   strbuf_splice(buf, p-- - buf->buf, 1, "", 0);
+
+   if (skip_prefix(buf->buf, "GIT_AUTHOR_NAME=", (const char **))) {
+   strbuf_splice(buf, 0, p - buf->buf, "", 0);
+   p = strchr(buf->buf, '\n');
+   if (skip_prefix(p, "\nGIT_AUTHOR_EMAIL=", (const char **))) {
+   strbuf_splice(buf, p - buf->buf, p2 - p, " <", 2);
+   p = strchr(p, '\n');
+   if (skip_prefix(p, "\nGIT_AUTHOR_DATE=@",
+   (const char **))) {
+   strbuf_splice(buf, p - buf->buf, p2 - p,
+ "> ", 2);
+   p = strchr(p, '\n');
+   if (p) {
+   strbuf_setlen(buf, p - buf->buf);
+   return buf->buf;
+   }
+   }
+   }
+   }
+
+   warning(_("could not parse '%s'"), rebase_path_author_script());
+   return NULL;
+}
+
 static const char staged_changes_advice[] =
 N_("you have staged changes in your working tree\n"
 "If these changes are meant to be squashed into the previous commit, run:\n"
@@ -711,6 +754,7 @@ N_("you have staged changes in your working tree\n"
 #define AMEND_MSG   (1<<2)
 #define CLEANUP_MSG (1<<3)
 #define VERIFY_MSG  (1<<4)
+#define CREATE_ROOT_COMMIT (1<<5)
 
 /*
  * If we are cherry-pick, and if the merge did not result in
@@ -730,6 +774,40 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
struct child_process cmd = CHILD_PROCESS_INIT;
const char *value;
 
+   if (flags & CREATE_ROOT_COMMIT) {
+   struct strbuf msg = STRBUF_INIT, script = STRBUF_INIT;
+   const char *author = is_rebase_i(opts) ?
+   read_author_ident() : NULL;
+   struct object_id root_commit, *cache_tree_oid;
+   int res = 0;
+
+   if (!defmsg)
+   BUG("root commit without message");
+
+   if 

[PATCH 1/6] sequencer: extract helper to update active_cache_tree

[Johannes Schindelin]

This patch extracts the code from is_index_unchanged() to initialize or
update the index' cache tree (i.e. a tree object reflecting the current
index' top-level tree).

The new helper will be used in the upcoming code to support `git rebase
-i --root` via the sequencer.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 27 ++-
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e2f83942843..90c8218aa9a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -562,9 +562,23 @@ static int do_recursive_merge(struct commit *base, struct 
commit *next,
return !clean;
 }
 
+static struct object_id *get_cache_tree_oid(void)
+{
+   if (!active_cache_tree)
+   active_cache_tree = cache_tree();
+
+   if (!cache_tree_fully_valid(active_cache_tree))
+   if (cache_tree_update(_index, 0)) {
+   error(_("unable to update cache tree"));
+   return NULL;
+   }
+
+   return _cache_tree->oid;
+}
+
 static int is_index_unchanged(void)
 {
-   struct object_id head_oid;
+   struct object_id head_oid, *cache_tree_oid;
struct commit *head_commit;
 
if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, _oid, NULL))
@@ -583,15 +597,10 @@ static int is_index_unchanged(void)
if (parse_commit(head_commit))
return -1;
 
-   if (!active_cache_tree)
-   active_cache_tree = cache_tree();
-
-   if (!cache_tree_fully_valid(active_cache_tree))
-   if (cache_tree_update(_index, 0))
-   return error(_("unable to update cache tree"));
+   if (!(cache_tree_oid = get_cache_tree_oid()))
+   return -1;
 
-   return !oidcmp(_cache_tree->oid,
-  _commit->tree->object.oid);
+   return !oidcmp(cache_tree_oid, _commit->tree->object.oid);
 }
 
 static int write_author_script(const char *message)
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 4/6] sequencer: allow introducing new root commits

[Johannes Schindelin]

In the context of the new --rebase-merges mode, which was designed
specifically to allow for changing the existing branch topology
liberally, a user may want to extract commits into a completely fresh
branch that starts with a newly-created root commit.

This is now possible by inserting the command `reset [new root]` before
`pick`ing the commit that wants to become a root commit. Example:

reset [new root]
pick 012345 a commit that is about to become a root commit
pick 234567 this commit will have the previous one as parent

This does not conflict with other uses of the `reset` command because
`[new root]` is not (part of) a valid ref name: both the opening bracket
as well as the space are illegal in ref names.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c  | 40 
 t/t3430-rebase-merges.sh | 34 ++
 2 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index fc124596b53..d10ebd62520 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2727,18 +2727,34 @@ static int do_reset(const char *name, int len, struct 
replay_opts *opts)
if (hold_locked_index(, LOCK_REPORT_ON_ERROR) < 0)
return -1;
 
-   /* Determine the length of the label */
-   for (i = 0; i < len; i++)
-   if (isspace(name[i]))
-   len = i;
-
-   strbuf_addf(_name, "refs/rewritten/%.*s", len, name);
-   if (get_oid(ref_name.buf, ) &&
-   get_oid(ref_name.buf + strlen("refs/rewritten/"), )) {
-   error(_("could not read '%s'"), ref_name.buf);
-   rollback_lock_file();
-   strbuf_release(_name);
-   return -1;
+   if (len == 10 && !strncmp("[new root]", name, len)) {
+   if (!opts->have_squash_onto) {
+   const char *hex;
+   if (commit_tree("", 0, the_hash_algo->empty_tree,
+   NULL, >squash_onto,
+   NULL, NULL))
+   return error(_("writing fake root commit"));
+   opts->have_squash_onto = 1;
+   hex = oid_to_hex(>squash_onto);
+   if (write_message(hex, strlen(hex),
+ rebase_path_squash_onto(), 0))
+   return error(_("writing squash-onto"));
+   }
+   oidcpy(, >squash_onto);
+   } else {
+   /* Determine the length of the label */
+   for (i = 0; i < len; i++)
+   if (isspace(name[i]))
+   len = i;
+
+   strbuf_addf(_name, "refs/rewritten/%.*s", len, name);
+   if (get_oid(ref_name.buf, ) &&
+   get_oid(ref_name.buf + strlen("refs/rewritten/"), )) {
+   error(_("could not read '%s'"), ref_name.buf);
+   rollback_lock_file();
+   strbuf_release(_name);
+   return -1;
+   }
}
 
memset(_tree_opts, 0, sizeof(unpack_tree_opts));
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 3d4dfdf7bec..35260862fcb 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -241,4 +241,38 @@ test_expect_success 'refuse to merge ancestors of HEAD' '
test_cmp_rev HEAD $before
 '
 
+test_expect_success 'root commits' '
+   git checkout --orphan unrelated &&
+   (GIT_AUTHOR_NAME="Parsnip" GIT_AUTHOR_EMAIL="r...@example.com" \
+test_commit second-root) &&
+   test_commit third-root &&
+   cat >script-from-scratch <<-\EOF &&
+   pick third-root
+   label first-branch
+   reset [new root]
+   pick second-root
+   merge first-branch # Merge the 3rd root
+   EOF
+   test_config sequence.editor \""$PWD"/replace-editor.sh\" &&
+   test_tick &&
+   git rebase -i --force --root -r &&
+   test "Parsnip" = "$(git show -s --format=%an HEAD^)" &&
+   test $(git rev-parse second-root^0) != $(git rev-parse HEAD^) &&
+   test $(git rev-parse second-root:second-root.t) = \
+   $(git rev-parse HEAD^:second-root.t) &&
+   test_cmp_graph HEAD <<-\EOF &&
+   *   Merge the 3rd root
+   |\
+   | * third-root
+   * second-root
+   EOF
+
+   : fast forward if possible &&
+   before="$(git rev-parse --verify HEAD)" &&
+   test_might_fail git config --unset sequence.editor &&
+   test_tick &&
+   git rebase -i --root -r &&
+   test_cmp_rev HEAD $before
+'
+
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH 0/6] Let the sequencer handle `git rebase -i --root`

[Johannes Schindelin]

When I reimplemented the most performance-critical bits of the
interactive rebase in the sequencer, to speed up `git rebase -i`
particularly on Windows (even if the benefits are still quite notable on
Linux or macOS), I punted on the --root part.

I had always hoped that some other contributor (or I myself) would come
back later to address the --root part in the sequencer, too, with the
idea that this would move the last remaining complicated code from
git-rebase--interactive.sh into sequencer.c, to facilitate converting
the rest of git-rebase--interactive.sh.

When I say "the last remaining complicated code", of course I neglect
the --preserve-merges code, but as I worked hard on the --rebase-merges
patch series with the intention to eventually deprecate and maybe even
remove the --preserve-merges mode, I always implicitly assume that the
--preserve-merges code will be moved into its own shell script
(git-rebase--preserve-merges.sh, maybe?) and never be converted.

So here goes: the patches to move the handling of --root into the
sequencer. After two preparatory patches, the real conversion takes
place in the third patch. After that, we take care of the --root related
concerns that arise in conjunction with the --rebase-merges mode.

As the --rebase-merges/--root patches overlap quite a bit (not so much
in the code itself as in philosophical considerations such as "what
should happen if you try to merge a branch into a new root", or the
fact that the label/reset/merge commands make it desirable to be able to
create a new root commit in the middle of a todo list), I had to
consider in which order to contribute them. In the end, I decided to go
with --rebase-merges first, so the --root patches are based on the
--rebase-merges patch series.

I consider this patch series a critical prerequisite for Alban's Google
Summer of Code project to convert rebase -i into a builtin.


Johannes Schindelin (6):
  sequencer: extract helper to update active_cache_tree
  sequencer: learn about the special "fake root commit" handling
  rebase -i --root: let the sequencer handle even the initial part
  sequencer: allow introducing new root commits
  rebase --rebase-merges: a "merge" into a new root is a fast-forward
  rebase --rebase-merges: root commits can be cousins, too

 git-rebase--interactive.sh|   4 +-
 sequencer.c   | 186 ++
 sequencer.h   |   4 +
 t/t3404-rebase-interactive.sh |  19 ++-
 t/t3421-rebase-topology-linear.sh |   6 +-
 t/t3430-rebase-merges.sh  |  72 
 6 files changed, 256 insertions(+), 35 deletions(-)


base-commit: 673fb9cb8b5c7d57cb560b6ade45e419c8dd09fc
Based-On: recreate-merges at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git recreate-merges
Published-As: 
https://github.com/dscho/git/releases/tag/sequencer-and-root-commits-v1
Fetch-It-Via: git fetch https://github.com/dscho/git 
sequencer-and-root-commits-v1
-- 
2.17.0.windows.1.33.gfcbb1fa0445

Re: BUG report: unicode normalization on APFS (Mac OS High Sierra)

[Totsten Bögershausen]

On 2018-04-26 19:23, Elijah Newren wrote:

On Thu, Apr 26, 2018 at 10:13 AM, Torsten Bögershausen  wrote:

Hm,
thanks for the report.
I don't have a high sierra box, but I can probably get one.
t0050 -should- pass automagically, so I feel that I can do something.
Unless someone is faster of course.


Sweet, thanks for taking a look.


Is it possible that  you run
debug=t verbose=t ./t0050-filesystem.sh
and send the output to me ?


Sure.  First, though, note that I can make it pass (or at least "not
ok...TODO known breakage") with the following patch (may be
whitespace-damaged by gmail):

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 483c8d6d7..770b91f8c 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1106,12 +1106,7 @@ test_lazy_prereq UTF8_NFD_TO_NFC '
 auml=$(printf "\303\244")
 aumlcdiar=$(printf "\141\314\210")
 >"$auml" &&
-   case "$(echo *)" in
-   "$aumlcdiar")
-   true ;;
-   *)
-   false ;;
-   esac
+   stat "$aumlcdiar" >/dev/null 2>/dev/null


Nicely analyzed and improved.

The "stat" statement is technically correct.
I think that a more git-style fix would be
[] ---
+   test -r "$aumlcdiar"

instead of the stat.

I looked into the 2 known breakages.
In short: they test use cases which are not sooo important for a user in 
practice, but do a good test if the code is broken.

IOW: I can't see a need for immediate action.

As you already did all the analyzes:
Do you want to send a patch ?

[PATCH v6 10/11] technical/shallow: describe why shallow cannot use replace refs

[Johannes Schindelin]

It is tempting to do away with commit_graft altogether (in the long
haul), now that grafts are deprecated.

However, the shallow feature needs a couple of things that the replace
refs cannot fulfill. Let's point that out in the documentation.

Signed-off-by: Johannes Schindelin 
---
 Documentation/technical/shallow.txt | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/technical/shallow.txt 
b/Documentation/technical/shallow.txt
index 4ec721335d2..01dedfe9ffe 100644
--- a/Documentation/technical/shallow.txt
+++ b/Documentation/technical/shallow.txt
@@ -17,6 +17,13 @@ Each line contains exactly one SHA-1. When read, a 
commit_graft
 will be constructed, which has nr_parent < 0 to make it easier
 to discern from user provided grafts.
 
+Note that the shallow feature could not be changed easily to
+use replace refs: a commit containing a `mergetag` is not allowed
+to be replaced, not even by a root commit. Such a commit can be
+made shallow, though. Also, having a `shallow` file explicitly
+listing all the commits made shallow makes it a *lot* easier to
+do shallow-specific things such as to deepen the history.
+
 Since fsck-objects relies on the library to read the objects,
 it honours shallow commits automatically.
 
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v6 11/11] Remove obsolete script to convert grafts to replace refs

[Johannes Schindelin]

The functionality is now implemented as `git replace
--convert-graft-file`.

Signed-off-by: Johannes Schindelin 
---
 contrib/convert-grafts-to-replace-refs.sh | 28 ---
 1 file changed, 28 deletions(-)
 delete mode 100755 contrib/convert-grafts-to-replace-refs.sh

diff --git a/contrib/convert-grafts-to-replace-refs.sh 
b/contrib/convert-grafts-to-replace-refs.sh
deleted file mode 100755
index 0cbc917b8cf..000
--- a/contrib/convert-grafts-to-replace-refs.sh
+++ /dev/null
@@ -1,28 +0,0 @@
-#!/bin/sh
-
-# You should execute this script in the repository where you
-# want to convert grafts to replace refs.
-
-GRAFTS_FILE="${GIT_DIR:-.git}/info/grafts"
-
-. $(git --exec-path)/git-sh-setup
-
-test -f "$GRAFTS_FILE" || die "Could not find graft file: '$GRAFTS_FILE'"
-
-grep '^[^# ]' "$GRAFTS_FILE" |
-while read definition
-do
-   if test -n "$definition"
-   then
-   echo "Converting: $definition"
-   git replace --graft $definition ||
-   die "Conversion failed for: $definition"
-   fi
-done
-
-mv "$GRAFTS_FILE" "$GRAFTS_FILE.bak" ||
-   die "Could not rename '$GRAFTS_FILE' to '$GRAFTS_FILE.bak'"
-
-echo "Success!"
-echo "All the grafts in '$GRAFTS_FILE' have been converted to replace refs!"
-echo "The grafts file '$GRAFTS_FILE' has been renamed: '$GRAFTS_FILE.bak'"
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v6 09/11] technical/shallow: stop referring to grafts

[Johannes Schindelin]

Now that grafts are deprecated, we should start to assume that readers
have no idea what grafts are. So it makes more sense to make the
description of the "shallow" feature stand on its own.

Suggested-by: Eric Sunshine 
Helped-by: Junio Hamano 
Signed-off-by: Johannes Schindelin 
---
 Documentation/technical/shallow.txt | 13 -
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/Documentation/technical/shallow.txt 
b/Documentation/technical/shallow.txt
index 5183b154229..4ec721335d2 100644
--- a/Documentation/technical/shallow.txt
+++ b/Documentation/technical/shallow.txt
@@ -8,15 +8,10 @@ repo, and therefore grafts are introduced pretending that
 these commits have no parents.
 *
 
-The basic idea is to write the SHA-1s of shallow commits into
-$GIT_DIR/shallow, and handle its contents like the contents
-of $GIT_DIR/info/grafts (with the difference that shallow
-cannot contain parent information).
-
-This information is stored in a new file instead of grafts, or
-even the config, since the user should not touch that file
-at all (even throughout development of the shallow clone, it
-was never manually edited!).
+$GIT_DIR/shallow lists commit object names and tells Git to
+pretend as if they are root commits (e.g. "git log" traversal
+stops after showing them; "git fsck" does not complain saying
+the commits listed on their "parent" lines do not exist).
 
 Each line contains exactly one SHA-1. When read, a commit_graft
 will be constructed, which has nr_parent < 0 to make it easier
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v6 07/11] Deprecate support for .git/info/grafts

[Johannes Schindelin]

The grafts feature was a convenient way to "stitch together" ancient
history to the fresh start of linux.git.

Its implementation is, however, not up to Git's standards, as there are
too many ways where it can lead to surprising and unwelcome behavior.

For example, when pushing from a repository with active grafts, it is
possible to miss commits that have been "grafted out", resulting in a
broken state on the other side.

Also, the grafts feature is limited to "rewriting" commits' list of
parents, it cannot replace anything else.

The much younger feature implemented as `git replace` set out to remedy
those limitations and dangerous bugs.

Seeing as `git replace` is pretty mature by now (since 4228e8bc98
(replace: add --graft option, 2014-07-19) it can perform the graft
file's duties), it is time to deprecate support for the graft file, and
to retire it eventually.

Signed-off-by: Johannes Schindelin 
Reviewed-by: Stefan Beller 
---
 advice.c  |  2 ++
 advice.h  |  1 +
 commit.c  | 10 ++
 t/t6001-rev-list-graft.sh |  9 +
 4 files changed, 22 insertions(+)

diff --git a/advice.c b/advice.c
index 406efc183ba..4411704fd45 100644
--- a/advice.c
+++ b/advice.c
@@ -19,6 +19,7 @@ int advice_rm_hints = 1;
 int advice_add_embedded_repo = 1;
 int advice_ignored_hook = 1;
 int advice_waiting_for_editor = 1;
+int advice_graft_file_deprecated = 1;
 
 static struct {
const char *name;
@@ -42,6 +43,7 @@ static struct {
{ "addembeddedrepo", _add_embedded_repo },
{ "ignoredhook", _ignored_hook },
{ "waitingforeditor", _waiting_for_editor },
+   { "graftfiledeprecated", _graft_file_deprecated },
 
/* make this an alias for backward compatibility */
{ "pushnonfastforward", _push_update_rejected }
diff --git a/advice.h b/advice.h
index 70568fa7922..9f5064e82a8 100644
--- a/advice.h
+++ b/advice.h
@@ -21,6 +21,7 @@ extern int advice_rm_hints;
 extern int advice_add_embedded_repo;
 extern int advice_ignored_hook;
 extern int advice_waiting_for_editor;
+extern int advice_graft_file_deprecated;
 
 int git_default_advice_config(const char *var, const char *value);
 __attribute__((format (printf, 1, 2)))
diff --git a/commit.c b/commit.c
index 2952ec987c5..451d3ce8dfe 100644
--- a/commit.c
+++ b/commit.c
@@ -12,6 +12,7 @@
 #include "prio-queue.h"
 #include "sha1-lookup.h"
 #include "wt-status.h"
+#include "advice.h"
 
 static struct commit_extra_header *read_commit_extra_header_lines(const char 
*buf, size_t len, const char **);
 
@@ -176,6 +177,15 @@ static int read_graft_file(const char *graft_file)
struct strbuf buf = STRBUF_INIT;
if (!fp)
return -1;
+   if (advice_graft_file_deprecated)
+   advise(_("Support for /info/grafts is deprecated\n"
+"and will be removed in a future Git version.\n"
+"\n"
+"Please use \"git replace --convert-graft-file\"\n"
+"to convert the grafts into replace refs.\n"
+"\n"
+"Turn this message off by running\n"
+"\"git config advice.graftFileDeprecated false\""));
while (!strbuf_getwholeline(, fp, '\n')) {
/* The format is just "Commit Parent1 Parent2 ...\n" */
struct commit_graft *graft = read_graft_line();
diff --git a/t/t6001-rev-list-graft.sh b/t/t6001-rev-list-graft.sh
index 05ddc69cf2a..7504ba47511 100755
--- a/t/t6001-rev-list-graft.sh
+++ b/t/t6001-rev-list-graft.sh
@@ -110,4 +110,13 @@ do
"
 
 done
+
+test_expect_success 'show advice that grafts are deprecated' '
+   git show HEAD 2>err &&
+   test_i18ngrep "git replace" err &&
+   test_config advice.graftFileDeprecated false &&
+   git show HEAD 2>err &&
+   test_i18ngrep ! "git replace" err
+'
+
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v6 08/11] filter-branch: stop suggesting to use grafts

[Johannes Schindelin]

The graft file is deprecated now, so let's use replace refs in the example
in filter-branch's man page instead.

Suggested-by: Eric Sunshine 
Signed-off-by: Johannes Schindelin 
---
 Documentation/git-filter-branch.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-filter-branch.txt 
b/Documentation/git-filter-branch.txt
index b634043183b..1d4d2f86045 100644
--- a/Documentation/git-filter-branch.txt
+++ b/Documentation/git-filter-branch.txt
@@ -288,7 +288,7 @@ git filter-branch --parent-filter \
 or even simpler:
 
 ---
-echo "$commit-id $graft-id" >> .git/info/grafts
+git replace --graft $commit-id $graft-id
 git filter-branch $graft-id..HEAD
 ---
 
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v6 02/11] commit: Let the callback of for_each_mergetag return on error

[Johannes Schindelin]

This is yet another patch to be filed under the keyword "libification".

There is one subtle change in behavior here, where a `git log` that has
been asked to show the mergetags would now stop reporting the mergetags
upon the first failure, whereas previously, it would have continued to the
next mergetag, if any.

In practice, that change should not matter, as it is 1) uncommon to
perform octopus merges using multiple tags as merge heads, and 2) when the
user asks to be shown those tags, they really should be there.

Signed-off-by: Johannes Schindelin 
---
 builtin/replace.c |  8 
 commit.c  |  8 +---
 commit.h  |  4 ++--
 log-tree.c| 13 +++--
 4 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 935647be6bd..245d3f4164e 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -345,7 +345,7 @@ struct check_mergetag_data {
const char **argv;
 };
 
-static void check_one_mergetag(struct commit *commit,
+static int check_one_mergetag(struct commit *commit,
   struct commit_extra_header *extra,
   void *data)
 {
@@ -368,20 +368,20 @@ static void check_one_mergetag(struct commit *commit,
if (get_oid(mergetag_data->argv[i], ) < 0)
die(_("Not a valid object name: '%s'"), 
mergetag_data->argv[i]);
if (!oidcmp(>tagged->oid, ))
-   return; /* found */
+   return 0; /* found */
}
 
die(_("original commit '%s' contains mergetag '%s' that is discarded; "
  "use --edit instead of --graft"), ref, oid_to_hex(_oid));
 }
 
-static void check_mergetags(struct commit *commit, int argc, const char **argv)
+static int check_mergetags(struct commit *commit, int argc, const char **argv)
 {
struct check_mergetag_data mergetag_data;
 
mergetag_data.argc = argc;
mergetag_data.argv = argv;
-   for_each_mergetag(check_one_mergetag, commit, _data);
+   return for_each_mergetag(check_one_mergetag, commit, _data);
 }
 
 static int create_graft(int argc, const char **argv, int force)
diff --git a/commit.c b/commit.c
index ca474a7c112..2952ec987c5 100644
--- a/commit.c
+++ b/commit.c
@@ -1288,17 +1288,19 @@ struct commit_extra_header 
*read_commit_extra_headers(struct commit *commit,
return extra;
 }
 
-void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
+int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
 {
struct commit_extra_header *extra, *to_free;
+   int res = 0;
 
to_free = read_commit_extra_headers(commit, NULL);
-   for (extra = to_free; extra; extra = extra->next) {
+   for (extra = to_free; !res && extra; extra = extra->next) {
if (strcmp(extra->key, "mergetag"))
continue; /* not a merge tag */
-   fn(commit, extra, data);
+   res = fn(commit, extra, data);
}
free_commit_extra_headers(to_free);
+   return res;
 }
 
 static inline int standard_header_field(const char *field, size_t len)
diff --git a/commit.h b/commit.h
index 0fb8271665c..9000895ad91 100644
--- a/commit.h
+++ b/commit.h
@@ -291,10 +291,10 @@ extern const char *find_commit_header(const char *msg, 
const char *key,
 /* Find the end of the log message, the right place for a new trailer. */
 extern int ignore_non_trailer(const char *buf, size_t len);
 
-typedef void (*each_mergetag_fn)(struct commit *commit, struct 
commit_extra_header *extra,
+typedef int (*each_mergetag_fn)(struct commit *commit, struct 
commit_extra_header *extra,
 void *cb_data);
 
-extern void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void 
*data);
+extern int for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void 
*data);
 
 struct merge_remote_desc {
struct object *obj; /* the named object, could be a tag */
diff --git a/log-tree.c b/log-tree.c
index d1c0bedf244..f3a51a6e726 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -488,9 +488,9 @@ static int is_common_merge(const struct commit *commit)
&& !commit->parents->next->next);
 }
 
-static void show_one_mergetag(struct commit *commit,
- struct commit_extra_header *extra,
- void *data)
+static int show_one_mergetag(struct commit *commit,
+struct commit_extra_header *extra,
+void *data)
 {
struct rev_info *opt = (struct rev_info *)data;
struct object_id oid;
@@ -502,7 +502,7 @@ static void show_one_mergetag(struct commit *commit,
hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), );
tag = lookup_tag();
if (!tag)
-   return; /* error message already given */
+   

[PATCH v6 04/11] replace: "libify" create_graft() and callees

[Johannes Schindelin]

File this away as yet another patch in the "libification" category.

As with all useful functions, in the next commit we want to use
create_graft() from a higher-level function where it would be
inconvenient if the called function simply die()s: if there is a
problem, we want to let the user know how to proceed, and the callee
simply has no way of knowing what to say.

Signed-off-by: Johannes Schindelin 
---
 builtin/replace.c | 169 ++
 1 file changed, 112 insertions(+), 57 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index e345a5a0f1c..e57d3d187ed 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -79,9 +79,9 @@ static int list_replace_refs(const char *pattern, const char 
*format)
else if (!strcmp(format, "long"))
data.format = REPLACE_FORMAT_LONG;
else
-   die("invalid replace format '%s'\n"
-   "valid formats are 'short', 'medium' and 'long'\n",
-   format);
+   return error("invalid replace format '%s'\n"
+"valid formats are 'short', 'medium' and 'long'\n",
+format);
 
for_each_replace_ref(show_reference, (void *));
 
@@ -134,7 +134,7 @@ static int delete_replace_ref(const char *name, const char 
*ref,
return 0;
 }
 
-static void check_ref_valid(struct object_id *object,
+static int check_ref_valid(struct object_id *object,
struct object_id *prev,
struct strbuf *ref,
int force)
@@ -142,12 +142,13 @@ static void check_ref_valid(struct object_id *object,
strbuf_reset(ref);
strbuf_addf(ref, "%s%s", git_replace_ref_base, oid_to_hex(object));
if (check_refname_format(ref->buf, 0))
-   die("'%s' is not a valid ref name.", ref->buf);
+   return error("'%s' is not a valid ref name.", ref->buf);
 
if (read_ref(ref->buf, prev))
oidclr(prev);
else if (!force)
-   die("replace ref '%s' already exists", ref->buf);
+   return error("replace ref '%s' already exists", ref->buf);
+   return 0;
 }
 
 static int replace_object_oid(const char *object_ref,
@@ -161,28 +162,33 @@ static int replace_object_oid(const char *object_ref,
struct strbuf ref = STRBUF_INIT;
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
+   int res = 0;
 
obj_type = oid_object_info(object, NULL);
repl_type = oid_object_info(repl, NULL);
if (!force && obj_type != repl_type)
-   die("Objects must be of the same type.\n"
-   "'%s' points to a replaced object of type '%s'\n"
-   "while '%s' points to a replacement object of type '%s'.",
-   object_ref, type_name(obj_type),
-   replace_ref, type_name(repl_type));
-
-   check_ref_valid(object, , , force);
+   return error("Objects must be of the same type.\n"
+"'%s' points to a replaced object of type '%s'\n"
+"while '%s' points to a replacement object of "
+"type '%s'.",
+object_ref, type_name(obj_type),
+replace_ref, type_name(repl_type));
+
+   if (check_ref_valid(object, , , force)) {
+   strbuf_release();
+   return -1;
+   }
 
transaction = ref_transaction_begin();
if (!transaction ||
ref_transaction_update(transaction, ref.buf, repl, ,
   0, NULL, ) ||
ref_transaction_commit(transaction, ))
-   die("%s", err.buf);
+   res = error("%s", err.buf);
 
ref_transaction_free(transaction);
strbuf_release();
-   return 0;
+   return res;
 }
 
 static int replace_object(const char *object_ref, const char *replace_ref, int 
force)
@@ -190,9 +196,11 @@ static int replace_object(const char *object_ref, const 
char *replace_ref, int f
struct object_id object, repl;
 
if (get_oid(object_ref, ))
-   die("Failed to resolve '%s' as a valid ref.", object_ref);
+   return error("Failed to resolve '%s' as a valid ref.",
+object_ref);
if (get_oid(replace_ref, ))
-   die("Failed to resolve '%s' as a valid ref.", replace_ref);
+   return error("Failed to resolve '%s' as a valid ref.",
+replace_ref);
 
return replace_object_oid(object_ref, , replace_ref, , 
force);
 }
@@ -202,7 +210,7 @@ static int replace_object(const char *object_ref, const 
char *replace_ref, int f
  * If "raw" is true, then the object's raw contents are printed according to
  * "type". Otherwise, we pretty-print the contents 

[PATCH v6 05/11] replace: introduce --convert-graft-file

[Johannes Schindelin]

This option is intended to help with the transition away from the
now-deprecated graft file.

Signed-off-by: Johannes Schindelin 
---
 Documentation/git-replace.txt | 11 ++---
 builtin/replace.c | 44 ++-
 2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index e5c57ae6ef4..246dc9943c2 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -11,6 +11,7 @@ SYNOPSIS
 'git replace' [-f]  
 'git replace' [-f] --edit 
 'git replace' [-f] --graft  [...]
+'git replace' [-f] --convert-graft-file
 'git replace' -d ...
 'git replace' [--format=] [-l []]
 
@@ -87,9 +88,13 @@ OPTIONS
content as  except that its parents will be
[...] instead of 's parents. A replacement ref
is then created to replace  with the newly created
-   commit. See contrib/convert-grafts-to-replace-refs.sh for an
-   example script based on this option that can convert grafts to
-   replace refs.
+   commit. Use `--convert-graft-file` to convert a
+   `$GIT_DIR/info/grafts` file and use replace refs instead.
+
+--convert-graft-file::
+   Creates graft commits for all entries in `$GIT_DIR/info/grafts`
+   and deletes that file upon success. The purpose is to help users
+   with transitioning off of the now-deprecated graft file.
 
 -l ::
 --list ::
diff --git a/builtin/replace.c b/builtin/replace.c
index e57d3d187ed..35394ec1874 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -20,6 +20,7 @@ static const char * const git_replace_usage[] = {
N_("git replace [-f]  "),
N_("git replace [-f] --edit "),
N_("git replace [-f] --graft  [...]"),
+   N_("git replace [-f] --convert-graft-file"),
N_("git replace -d ..."),
N_("git replace [--format=] [-l []]"),
NULL
@@ -476,6 +477,38 @@ static int create_graft(int argc, const char **argv, int 
force)
return replace_object_oid(old_ref, _oid, "replacement", _oid, 
force);
 }
 
+static int convert_graft_file(int force)
+{
+   const char *graft_file = get_graft_file();
+   FILE *fp = fopen_or_warn(graft_file, "r");
+   struct strbuf buf = STRBUF_INIT, err = STRBUF_INIT;
+   struct argv_array args = ARGV_ARRAY_INIT;
+
+   if (!fp)
+   return -1;
+
+   while (strbuf_getline(, fp) != EOF) {
+   if (*buf.buf == '#')
+   continue;
+
+   argv_array_split(, buf.buf);
+   if (args.argc && create_graft(args.argc, args.argv, force))
+   strbuf_addf(, "\n\t%s", buf.buf);
+   argv_array_clear();
+   }
+   fclose(fp);
+
+   strbuf_release();
+
+   if (!err.len)
+   return unlink_or_warn(graft_file);
+
+   warning(_("could not convert the following graft(s):\n%s"), err.buf);
+   strbuf_release();
+
+   return -1;
+}
+
 int cmd_replace(int argc, const char **argv, const char *prefix)
 {
int force = 0;
@@ -487,6 +520,7 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
MODE_DELETE,
MODE_EDIT,
MODE_GRAFT,
+   MODE_CONVERT_GRAFT_FILE,
MODE_REPLACE
} cmdmode = MODE_UNSPECIFIED;
struct option options[] = {
@@ -494,6 +528,7 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
OPT_CMDMODE('d', "delete", , N_("delete replace refs"), 
MODE_DELETE),
OPT_CMDMODE('e', "edit", , N_("edit existing object"), 
MODE_EDIT),
OPT_CMDMODE('g', "graft", , N_("change a commit's 
parents"), MODE_GRAFT),
+   OPT_CMDMODE(0, "convert-graft-file", , N_("convert 
existing graft file"), MODE_CONVERT_GRAFT_FILE),
OPT_BOOL_F('f', "force", , N_("replace the ref if it 
exists"),
   PARSE_OPT_NOCOMPLETE),
OPT_BOOL(0, "raw", , N_("do not pretty-print contents for 
--edit")),
@@ -516,7 +551,8 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
if (force &&
cmdmode != MODE_REPLACE &&
cmdmode != MODE_EDIT &&
-   cmdmode != MODE_GRAFT)
+   cmdmode != MODE_GRAFT &&
+   cmdmode != MODE_CONVERT_GRAFT_FILE)
usage_msg_opt("-f only makes sense when writing a replacement",
  git_replace_usage, options);
 
@@ -549,6 +585,12 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
  git_replace_usage, options);
return create_graft(argc, argv, force);
 
+   case MODE_CONVERT_GRAFT_FILE:
+   if (argc != 0)
+   usage_msg_opt("--convert-graft-file takes no argument",
+ git_replace_usage, options);
+   return 

[PATCH v6 01/11] argv_array: offer to split a string by whitespace

[Johannes Schindelin]

This is a simple function that will interpret a string as a whitespace
delimited list of values, and add those values into the array.

Note: this function does not (yet) offer to split by arbitrary delimiters,
or keep empty values in case of runs of whitespace, or de-quote Unix shell
style. All fo this functionality can be added later, when and if needed.

Signed-off-by: Johannes Schindelin 
---
 argv-array.c | 20 
 argv-array.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/argv-array.c b/argv-array.c
index 5d370fa3366..cb5bcd2c064 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -64,6 +64,26 @@ void argv_array_pop(struct argv_array *array)
array->argc--;
 }
 
+void argv_array_split(struct argv_array *array, const char *to_split)
+{
+   while (isspace(*to_split))
+   to_split++;
+   for (;;) {
+   const char *p = to_split;
+
+   if (!*p)
+   break;
+
+   while (*p && !isspace(*p))
+   p++;
+   argv_array_push_nodup(array, xstrndup(to_split, p - to_split));
+
+   while (isspace(*p))
+   p++;
+   to_split = p;
+   }
+}
+
 void argv_array_clear(struct argv_array *array)
 {
if (array->argv != empty_argv) {
diff --git a/argv-array.h b/argv-array.h
index 29056e49a12..750c30d2f2c 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -19,6 +19,8 @@ LAST_ARG_MUST_BE_NULL
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pushv(struct argv_array *, const char **);
 void argv_array_pop(struct argv_array *);
+/* Splits by whitespace; does not handle quoted arguments! */
+void argv_array_split(struct argv_array *, const char *);
 void argv_array_clear(struct argv_array *);
 const char **argv_array_detach(struct argv_array *);
 
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v6 00/11] Deprecate .git/info/grafts

[Johannes Schindelin]

It is fragile, as there is no way for the revision machinery to say "but
now I want to traverse the graph ignoring the graft file" e.g. when
pushing commits to a remote repository (which, as a consequence, can
miss commits).

And we already have a better solution with `git replace --graft 
[...]`.

Changes since v5:

- Disentangled the lumped-together conditional blocks in
  edit_and_replace() again.

- Moved fixup (a superfluous argv_array_clear()) from the patch that
  adds a test for --convert-graft-file back to the patch that actually
  introduces that option.


Johannes Schindelin (11):
  argv_array: offer to split a string by whitespace
  commit: Let the callback of for_each_mergetag return on error
  replace: avoid using die() to indicate a bug
  replace: "libify" create_graft() and callees
  replace: introduce --convert-graft-file
  Add a test for `git replace --convert-graft-file`
  Deprecate support for .git/info/grafts
  filter-branch: stop suggesting to use grafts
  technical/shallow: stop referring to grafts
  technical/shallow: describe why shallow cannot use replace refs
  Remove obsolete script to convert grafts to replace refs

 Documentation/git-filter-branch.txt   |   2 +-
 Documentation/git-replace.txt |  11 +-
 Documentation/technical/shallow.txt   |  20 +-
 advice.c  |   2 +
 advice.h  |   1 +
 argv-array.c  |  20 ++
 argv-array.h  |   2 +
 builtin/replace.c | 223 --
 commit.c  |  18 +-
 commit.h  |   4 +-
 contrib/convert-grafts-to-replace-refs.sh |  28 ---
 log-tree.c|  13 +-
 t/t6001-rev-list-graft.sh |   9 +
 t/t6050-replace.sh|  20 ++
 14 files changed, 258 insertions(+), 115 deletions(-)
 delete mode 100755 contrib/convert-grafts-to-replace-refs.sh


base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d
Published-As: https://github.com/dscho/git/releases/tag/deprecate-grafts-v6
Fetch-It-Via: git fetch https://github.com/dscho/git deprecate-grafts-v6

Interdiff vs v5:
 diff --git a/builtin/replace.c b/builtin/replace.c
 index acd30e3d824..35394ec1874 100644
 --- a/builtin/replace.c
 +++ b/builtin/replace.c
 @@ -326,10 +326,15 @@ static int edit_and_replace(const char *object_ref, int 
force, int raw)
strbuf_release();
  
tmpfile = git_pathdup("REPLACE_EDITOBJ");
 -  if (export_object(_oid, type, raw, tmpfile) ||
 -  (launch_editor(tmpfile, NULL, NULL) < 0 &&
 -   error("editing object file failed")) ||
 -  import_object(_oid, type, raw, tmpfile)) {
 +  if (export_object(_oid, type, raw, tmpfile)) {
 +  free(tmpfile);
 +  return -1;
 +  }
 +  if (launch_editor(tmpfile, NULL, NULL) < 0) {
 +  free(tmpfile);
 +  return error("editing object file failed");
 +  }
 +  if (import_object(_oid, type, raw, tmpfile)) {
free(tmpfile);
return -1;
}
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v6 06/11] Add a test for `git replace --convert-graft-file`

[Johannes Schindelin]

The proof, as the saying goes, lies in the pudding. So here is a
regression test that not only demonstrates what the option is supposed to
accomplish, but also demonstrates that it does accomplish it.

Signed-off-by: Johannes Schindelin 
---
 t/t6050-replace.sh | 20 
 1 file changed, 20 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index c630aba657e..bed86a0af3d 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -444,4 +444,24 @@ test_expect_success GPG '--graft on a commit with a 
mergetag' '
git replace -d $HASH10
 '
 
+test_expect_success '--convert-graft-file' '
+   : add and convert graft file &&
+   printf "%s\n%s %s\n\n# comment\n%s\n" \
+   $(git rev-parse HEAD^^ HEAD^ HEAD^^ HEAD^2) \
+   >.git/info/grafts &&
+   git replace --convert-graft-file &&
+   test_path_is_missing .git/info/grafts &&
+
+   : verify that the history is now "grafted" &&
+   git rev-list HEAD >out &&
+   test_line_count = 4 out &&
+
+   : create invalid graft file and verify that it is not deleted &&
+   test_when_finished "rm -f .git/info/grafts" &&
+   echo $EMPTY_BLOB $EMPTY_TREE >.git/info/grafts &&
+   test_must_fail git replace --convert-graft-file 2>err &&
+   test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" err &&
+   test_i18ngrep "$EMPTY_BLOB $EMPTY_TREE" .git/info/grafts
+'
+
 test_done
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v6 03/11] replace: avoid using die() to indicate a bug

[Johannes Schindelin]

We have the BUG() macro for that purpose.

Signed-off-by: Johannes Schindelin 
---
 builtin/replace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 245d3f4164e..e345a5a0f1c 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
return list_replace_refs(argv[0], format);
 
default:
-   die("BUG: invalid cmdmode %d", (int)cmdmode);
+   BUG("invalid cmdmode %d", (int)cmdmode);
}
 }
-- 
2.17.0.windows.1.33.gfcbb1fa0445

Re: [PATCH 3/3] rebase --skip: clean up commit message after a failedfixup/squash

[Johannes Schindelin]

Hi Phillip,

On Sat, 21 Apr 2018, Phillip Wood wrote:

> On 20/04/18 13:18, Johannes Schindelin wrote:
> > 
> > During a series of fixup/squash commands, the interactive rebase builds
> > up a commit message with comments. This will be presented to the user in
> > the editor if at least one of those commands was a `squash`.
> > 
> > However, if the last of these fixup/squash commands fails with merge
> > conflicts, and if the user then decides to skip it (or resolve it to a
> > clean worktree and then continue the rebase), the current code fails to
> > clean up the commit message.
> 
> Thanks for taking the time to track this down and fix it.

It was on my mind, but since I got caught twice by this bug within a week,
I figured it was about time.

> > diff --git a/sequencer.c b/sequencer.c
> > index a9c3bc26f84..f067b7b24c5 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2781,17 +2781,12 @@ static int continue_single_pick(void)
> >   
> >   static int commit_staged_changes(struct replay_opts *opts)
> >   {
> > -   unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
> > +   unsigned int flags = ALLOW_EMPTY | EDIT_MSG, is_fixup = 0, is_clean;
> >   
> >if (has_unstaged_changes(1))
> > return error(_("cannot rebase: You have unstaged changes."));
> > -   if (!has_uncommitted_changes(0)) {
> > -   const char *cherry_pick_head = git_path_cherry_pick_head();
> >   - if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
> > -   return error(_("could not remove CHERRY_PICK_HEAD"));
> > -   return 0;
> > -   }
> > +   is_clean = !has_uncommitted_changes(0);
> >   
> >if (file_exists(rebase_path_amend())) {
> > struct strbuf rev = STRBUF_INIT;
> > @@ -2804,16 +2799,41 @@ static int commit_staged_changes(struct replay_opts
> > *opts)
> > if (get_oid_hex(rev.buf, _amend))
> >  return error(_("invalid contents: '%s'"),
> > rebase_path_amend());
> > -   if (oidcmp(, _amend))
> > +   if (!is_clean && oidcmp(, _amend))
> >  return error(_("\nYou have uncommitted changes in your "
> >  "working tree. Please, commit them\n"
> >  "first and then run 'git rebase "
> >  "--continue' again."));
> > +   if (is_clean && !oidcmp(, _amend)) {
> 
> Looking at pick_commits() it only writes to rebase_path_amend() if there are
> conflicts, not if the command has been rescheduled so this is safe.

This is indeed the intent of that file.

> > +   strbuf_reset();
> > +   /*
> > +* Clean tree, but we may need to finalize a
> > +* fixup/squash chain. A failed fixup/squash leaves
> > the
> > +* file amend-type in rebase-merge/; It is okay if
> > that
> > +* file is missing, in which case there is no such
> > +* chain to finalize.
> > +*/
> > +   read_oneliner(, rebase_path_amend_type(), 0);
> > +   if (!strcmp("squash", rev.buf))
> > +   is_fixup = TODO_SQUASH;
> > +   else if (!strcmp("fixup", rev.buf)) {
> > +   is_fixup = TODO_FIXUP;
> > +   flags = (flags & ~EDIT_MSG) | CLEANUP_MSG;
> 
> I was going to say this should probably be (flags & ~(EDIT_MSG | VERIFY_MSG))
> but for some reason VERIFY_MSG isn't set here - I wonder if it should be as I
> think it's set elsewhere when we edit the message.

As this patch series is purely about the bug fix where interrupted
fixup/squash series can lead to incorrect commit messages, I would say
that if this is a bug, it should be fixed in a separate patch series.

The name of that option is actually a little bit unfortunate: it bypasses
the pre-commit/commit-msg hooks. I am not sure why they are bypassed in
the commit_staged_changes() function.

*clickety-click*

It would appear that I simply copied this from

https://github.com/git/git/blob/v2.17.0/git-rebase--interactive.sh#L794-L808

So where does that come from? Let's use `git log -L
794,808:git-rebase--interactive.sh v2.17.0` to find out.

*clickety-click*

>From that log, it looks as if this was added in 2147f844ed1 (rebase -i: handle
fixup of root commit correctly, 2012-07-24). But that is incorrect: the
--no-verify invocation was only split into two by said commit, and moved
into the conditional. So we need to look a little further, with a larger
line range (I extended it to 810 for the purpose of this analysis).

*clickety-click*

So it goes all the way back to c5b09feb786 (Avoid update hook during
git-rebase --interactive, 2007-12-19). From that commit message, you can
see that the rationale for the --no-verify flag was as following: the `git
commit` might fail when continuing with staged changes, due to a check in
a hook, and since there is inadequate error checking in the

Re: [PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Stefan Beller]

On Fri, Apr 27, 2018 at 1:48 PM, Johannes Schindelin
 wrote:
> During a series of fixup/squash commands, the interactive rebase builds
> up a commit message with comments. This will be presented to the user in
> the editor if at least one of those commands was a `squash`.

This sounds as if the whole series will be presented to the user, i.e.

 pick A
 squash B
 fixup C

would present A+B+C in the editor. I always assumed the sequencer
to be linear, i.e. pick A+B, open editor and then fixup C into the
previous result?

No need to resend it reworded, I just realize that I never tested my
potentially wrong assumption.

> The diff is best viewed with --color-moved.

... and web pages are "best viewed with IE 6.0" ;-)

I found this so funny that I had to download the patches and actually
look at them
using the move detection only to find out that only very few lines are moved,
as there are only very few deleted lines.

Re: [PATCH 18/41] index-pack: abstract away hash function constant

[brian m. carlson]

On Thu, Apr 26, 2018 at 05:46:28PM +0200, Duy Nguyen wrote:
> On Wed, Apr 25, 2018 at 8:49 PM, Martin Ågren  wrote:
> > Once that is accomplished, I sort of suspect that this code will want to
> > be updated to not always blindly use the_hash_algo, but to always work
> > with SHA-1 sizes. Or rather, this would turn into more generic code to
> > handle both "v2 with SHA-1" and "v3 with some hash function(s)". This
> > commit might be a good first step in that direction.
> 
> I also have an uneasy feeling when things this close to on-disk file
> format get hash-agnostic treatment. I think we would need to start
> adding new file formats soon, from bottom up with simple things like
> loose object files (cat-file and hash-object should be enough to test
> blobs...), then moving up to pack files and more. This is when we can
> really decide where to use the new hash and whether we should keep
> some hashes as sha-1.

I agree that this is work which needs to be done soon.  There are
basically a couple of pieces we need to handle NewHash:

* Remove the dependencies on SHA-1 as much as possible.
* Get the tests to pass with a different hash (almost done for 160-bit
  hash; in progress for 256-bit hashes).
* Write pack code.
* Write loose object index code.
* Write read-as-SHA-1 code.
* Force the codebase to always use SHA-1 when dealing with fetch/push.
* Distinguish between code which needs to use compatObjectFormat and
  code which needs to use objectFormat.
* Decide on NewHash.

I'm working on the top two bullet points right now.  Others are welcome
to pick up other pieces, or I'll get to them eventually.

As much as I'm dreading having the bikeshedding discussion over what
we're going to pick for NewHash, some of these pieces require knowing
what algorithm it will be.  For example, we have some tests which either
need to be completely rewritten or have a translation table written for
them (think the ones that use colliding short names).  In order for
those tests to have the translation table written, we need to be able to
compute colliding values.  I'm annotating these with prerequisites, but
there are quite a few tests which are skipped.

I expect writing the pack, loose object index, and read-as-SHA-1 code is
going to require having some code for NewHash or stand-in present in
order for it to compile and be tested.  It's possible that others could
come up with more imaginative solutions that don't require that, but I
have my doubts.

> For trailing hashes for example, there's no need to move to a new hash
> which only costs us more cycles. We just use it as a fancy checksum to
> avoid bit flips. But then my assumption about cost may be completely
> wrong without experimenting.

I would argue that consistency is helpful.  Also, do we really want
people to be able to (eventually) create colliding packs that contain
different data?  That doesn't seem like a good idea.

But also, some of the candidates we're considering for NewHash are
actually faster than SHA-1.  So for performance reasons alone, it might
be useful to adopt a consistent scheme.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [RFC PATCH] checkout: Force matching mtime between files

[Marc Branchaud]

On 2018-04-27 01:03 PM, Duy Nguyen wrote:

On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud  wrote:

The best approach to do so is to have those people do the "touch"
thing in their own post-checkout hook.  People who use Git as the
source control system won't have to pay runtime cost of doing the
touch thing, and we do not have to maintain such a hook script.
Only those who use the "feature" would.



The post-checkout hook approach is not exactly straightforward.


I am revisiting this because I'm not even happy with my
post-checkout-modified hook suggestion, so..



Naively, it's simply

 for F in `git diff --name-only $1 $2`; do touch "$F"; done

But consider:

* Symlinks can cause the wrong file to be touched.  (Granted, Michał's
proposed patch also doesn't deal with symlinks.)  Let's assume that a hook
can be crafted will all possible sophistication.  There are still some
fundamental problems:


OK so this one could be tricky to get right, but it's possible.



* In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
identical so the above loop does nothing.  Offhand I'm not even sure how a
hook might get the right files in this case.


This is a limitation of the current post-checkout hook. $3==0 from the
hook lets us know this is not a branch switch, but it does not really
tell you the affected paths. If it somehow passes all the given
pathspec to you, then you should be able to do "git ls-files --
$pathspec" which gives you the exact same set of paths that
git-checkout updates. We could do this by setting $4 to "--" and put
all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
the above example.

There is  third case here, if you do "git checkout  --
path/to/file" then it cannot be covered by the current design. I guess
we could set $3 to '2' (retrieve from a tree) to indicate this in
addition to 0 (from index) and 1 (from switching branch) and then $1
could be the tree in question (pathspecs are passed the same way
above)

[1] I wonder if we could have a more generic approach to pass
pathspecs via environment, which could work for more than just this
one hook. Not sure if it's a good idea though.


I think there needs to be something other than listing all the paths in 
the command is viable, because it's too easy to hit some 
command-line-length limit.  It would also be good if hook authors didn't 
have to re-invent the wheel of determining the changed paths for every 
corner-case.


My first instinct is to write them one-per-line on the hook's stdin. 
That's probably not generic enough for most hooks, but it seems like a 
good approach for this proposal.


Throwing them into a temporary file with a known name is also good --- 
better, I think, than stuffing them into an environment variable.


M.


* The hook has to be set up in every repo and submodule (at least until
something like Ævar's experiments come to fruition).


Either --template or core.hooksPath would solve this, or I'll try to
get my "hooks.* config" patch in. I think that one is a good thing to
do anyway because it allows more flexible hook management (and it
could allow multiple hooks of the same name too). With this, we could
avoid adding more command-specific config like core.fsmonitor or
uploadpack.packObjectsHook which to me are hooks.

Another option is extend core.hooksPath for searching multiple places
instead of just one like AEvar suggested.





* A fresh clone can't run the hook.  This is especially important when
dealing with submodules.  (In one case where we were bit by this, make
though that half of a fresh submodule clone's files were stale, and decided
to re-autoconf the entire thing.)


Both --template and config-based hooks should let you handle this case.

So, I think if we improve the hook system to give more information
(which is something we definitely should do), we could do this without
adding special cases in git. I'm not saying that we should never add
special cases, but at least this lets us play with different kinds of
post-checkout activities before we decide which one would be best
implemented in git.

Re: [RFC PATCH] checkout: Force matching mtime between files

[Elijah Newren]

On Fri, Apr 27, 2018 at 10:03 AM, Duy Nguyen  wrote:
> On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud  wrote:
>>
>> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
>> identical so the above loop does nothing.  Offhand I'm not even sure how a
>> hook might get the right files in this case.
>
> This is a limitation of the current post-checkout hook. $3==0 from the
> hook lets us know this is not a branch switch, but it does not really
> tell you the affected paths. If it somehow passes all the given
> pathspec to you, then you should be able to do "git ls-files --
> $pathspec" which gives you the exact same set of paths that
> git-checkout updates. We could do this by setting $4 to "--" and put
> all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
> the above example.
>
> There is  third case here, if you do "git checkout  --
> path/to/file" then it cannot be covered by the current design. I guess
> we could set $3 to '2' (retrieve from a tree) to indicate this in
> addition to 0 (from index) and 1 (from switching branch) and then $1
> could be the tree in question (pathspecs are passed the same way
> above)
>
> [1] I wonder if we could have a more generic approach to pass
> pathspecs via environment, which could work for more than just this
> one hook. Not sure if it's a good idea though.

Here's a crazy idea -- maybe instead of a list of pathspecs you just
provide the timestamp of when git checkout started.  Then the hook
could walk the tree, find all files with modification times at least
that late, and modify them all back to the the timestamp of when the
git checkout started.

Would that be enough?  Is that too crazy?

Sure, people could concurrently edit a file or run another program
that modified files, but if you're doing that you're already playing
race games with whether your next incremental build is going to be
able to be correct.  (Some (annoying) IDEs explicitly lock you out
from editing files during a build to attempt to avoid this very
problem.)

That does leave one other caveat: If people intentionally do really
weird stuff with having files with modification timestamps far in the
future.  However, it seems likely that the group of people doing that,
if non-zero in number, is likely to be dis-joint with the group of
folks that want this special
uniform-timestamp-across-files-in-a-checkout behavior.

Re: [PATCH v5 00/11] Deprecate .git/info/grafts

[Johannes Schindelin]

Hi Junio,

On Thu, 26 Apr 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >  -  if (export_object(_oid, type, raw, tmpfile))
> >  -  return -1;
> >  -  if (launch_editor(tmpfile, NULL, NULL) < 0)
> >  -  return error("editing object file failed");
> >  -  if (import_object(_oid, type, raw, tmpfile))
> >  +  tmpfile = git_pathdup("REPLACE_EDITOBJ");
> >  +  if (export_object(_oid, type, raw, tmpfile) ||
> >  +  (launch_editor(tmpfile, NULL, NULL) < 0 &&
> >  +   error("editing object file failed")) ||
> >  +  import_object(_oid, type, raw, tmpfile)) {
> >  +  free(tmpfile);
> > return -1;
> >  -
> >  +  }
> 
> I know the above is to avoid leaking tmpfile, but a single if ()
> condition that makes multiple calls to functions primarily for their
> side effects is too ugly to live.

I changed it back to individual conditional blocks, with every single one
of them having their own free(tmpfile). That is at least clearer.

Ciao,
Dscho

[PATCH v4 1/4] rebase -i: demonstrate bugs with fixup!/squash! commit messages

[Johannes Schindelin]

When multiple fixup/squash commands are processed and the last one
causes merge conflicts and is skipped, we leave the "This is a
combination of ..." comments in the commit message.

Noticed by Eric Sunshine.

This regression test also demonstrates that we rely on the localized
version of

# This is a combination of  commits

to contain the  in ASCII, which breaks under GETTEXT_POISON.

Signed-off-by: Johannes Schindelin 
---
 t/t3418-rebase-continue.sh | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 9214d0bb511..3874f187246 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -88,6 +88,28 @@ test_expect_success 'rebase passes merge strategy options 
correctly' '
git rebase --continue
 '
 
+test_expect_failure '--skip after failed fixup cleans commit message' '
+   test_when_finished "test_might_fail git rebase --abort" &&
+   git checkout -b with-conflicting-fixup &&
+   test_commit wants-fixup &&
+   test_commit "fixup! wants-fixup" wants-fixup.t 1 wants-fixup-1 &&
+   test_commit "fixup! wants-fixup" wants-fixup.t 2 wants-fixup-2 &&
+   test_commit "fixup! wants-fixup" wants-fixup.t 3 wants-fixup-3 &&
+   test_must_fail env FAKE_LINES="1 fixup 2 fixup 4" \
+   git rebase -i HEAD~4 &&
+
+   : now there is a conflict, and comments in the commit message &&
+   git show HEAD >out &&
+   grep "fixup! wants-fixup" out &&
+
+   : skip and continue &&
+   git rebase --skip &&
+
+   : now the comments in the commit message should have been cleaned up &&
+   git show HEAD >out &&
+   ! grep "fixup! wants-fixup" out
+'
+
 test_expect_success 'setup rerere database' '
rm -fr .git/rebase-* &&
git reset --hard commit-new-file-F3-on-topic-branch &&
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v4 0/4] rebase -i: avoid stale "# This is a combination of" in commit messages

[Johannes Schindelin]

Eric Sunshine pointed out that I had such a commit message in
https://public-inbox.org/git/CAPig+cRrS0_nYJJY=o6cbov630snqhpv5qgrqdd8mw-syzn...@mail.gmail.com/
and I went on a hunt to figure out how the heck this happened.

Turns out that if there is a fixup/squash chain where the *last* command
fails with merge conflicts, and we either --skip ahead or resolve the
conflict to a clean tree and then --continue, our code does not do a
final cleanup.

Contrary to my initial gut feeling, this bug was not introduced by my
rewrite in C of the core parts of rebase -i, but it looks to me as if
that bug was with us for a very long time (at least the --skip part).

The developer (read: user of rebase -i) in me says that we would want to
fast-track this, but the author of rebase -i in me says that we should
be cautious and cook this in `next` for a while.

Fixes since v3 (thanks, Phillip, for the really fruitful discussion!):

- We now avoid using the commit message prepared for the skipped
  fixup/squash.

- Replaced the "rebase -i: Handle "combination of  commits" with
  GETTEXT_POISON" patch by a *real* fix instead of a work-around: Instead
  of parsing the first line of the commit message and punting when it is
  missing an ASCII-encoded number, we determine  separately
  (independent from any localized text).

- Fixed quite a couple more corner cases, using the `current-fixups`
  file introduced for the GETTEXT_POISON fix:

  * we only need to re-commit if this was the final fixup/squash in the
fixup/squash chain,

  * we only need to commit interactively if there was *any* non-skipped
squash,

  * if the fixup/squash chain continues, the  was incorrect in the
"This is a combination of  commits" comment in the intermediate
commit message (it included the now-skipped commits), and

  * even if a filed fixup/squash in the middle of a fixup/squash chain
failed, and its merge conflicts were resolved and committed, the
"This is a combination of  commits" comment was incorrect: we
had already deleted message-fixup and message-squash, so the next
update_squash_message() would mistakenly assume that we were
starting afresh. Worse: if only fixup commands were remaining, but
there had been a squash command, we would retain the "squash!" line
in the commit message and not give the user a chance to clean things
up in the final fixup!


Johannes Schindelin (4):
  rebase -i: demonstrate bugs with fixup!/squash! commit messages
  rebase -i: Handle "combination of  commits" with GETTEXT_POISON
  sequencer: always commit without editing when asked for
  rebase --skip: clean up commit message after a failed fixup/squash

 sequencer.c| 193 -
 sequencer.h|   6 +-
 t/t3418-rebase-continue.sh |  49 ++
 3 files changed, 200 insertions(+), 48 deletions(-)


base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d
Published-As: 
https://github.com/dscho/git/releases/tag/clean-msg-after-fixup-continue-v4
Fetch-It-Via: git fetch https://github.com/dscho/git 
clean-msg-after-fixup-continue-v4

Interdiff vs v3:
 diff --git a/sequencer.c b/sequencer.c
 index e1efb0ebf31..cec180714ef 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -74,13 +74,6 @@ static GIT_PATH_FUNC(rebase_path_message, 
"rebase-merge/message")
   * previous commit and from the first squash/fixup commit are written
   * to it. The commit message for each subsequent squash/fixup commit
   * is appended to the file as it is processed.
 - *
 - * The first line of the file is of the form
 - * # This is a combination of $count commits.
 - * where $count is the number of commits whose messages have been
 - * written to the file so far (including the initial "pick" commit).
 - * Each time that a commit message is processed, this line is read and
 - * updated. It is deleted just before the combined commit is made.
   */
  static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash")
  /*
 @@ -91,6 +84,11 @@ static GIT_PATH_FUNC(rebase_path_squash_msg, 
"rebase-merge/message-squash")
   * commit without opening the editor.)
   */
  static GIT_PATH_FUNC(rebase_path_fixup_msg, "rebase-merge/message-fixup")
 +/*
 + * This file contains the list fixup/squash commands that have been
 + * accumulated into message-fixup or message-squash so far.
 + */
 +static GIT_PATH_FUNC(rebase_path_current_fixups, 
"rebase-merge/current-fixups")
  /*
   * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
   * GIT_AUTHOR_DATE that will be used for the commit that is currently
 @@ -106,13 +104,6 @@ static GIT_PATH_FUNC(rebase_path_author_script, 
"rebase-merge/author-script")
   * command is processed, this file is deleted.
   */
  static GIT_PATH_FUNC(rebase_path_amend, "rebase-merge/amend")
 -/*
 - * If there was a merge conflict in a fixup/squash series, we need to
 - * record the type so that a `git rebase --skip` can clean up the commit
 - * message as 

[PATCH v4 3/4] sequencer: always commit without editing when asked for

[Johannes Schindelin]

Previously, we only called run_git_commit() without EDIT_MSG when we also
passed in a default message.

However, an upcoming caller will want to commit without EDIT_MSG and
*without* a default message: to clean up fixup/squash comments in HEAD's
commit message.

Let's prepare for that.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index d2e6f33023d..56166b0d6c7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -717,6 +717,8 @@ static int run_git_commit(const char *defmsg, struct 
replay_opts *opts,
argv_array_pushf(, "-S%s", opts->gpg_sign);
if (defmsg)
argv_array_pushl(, "-F", defmsg, NULL);
+   else if (!(flags & EDIT_MSG))
+   argv_array_pushl(, "-C", "HEAD", NULL);
if ((flags & CLEANUP_MSG))
argv_array_push(, "--cleanup=strip");
if ((flags & EDIT_MSG))
-- 
2.17.0.windows.1.33.gfcbb1fa0445

[PATCH v4 2/4] rebase -i: Handle "combination of commits" with GETTEXT_POISON

[Johannes Schindelin]

We previously relied on the localized versions of

# This is a combination of  commits

(which we write into the commit messages during fixup/squash chains)
to contain  encoded in ASCII.

This is not true in general, and certainly not true when compiled with
GETTEXT_POISON=TryToKillMe, as demonstrated by the regression test we
just introduced in t3418.

So let's decouple keeping track of the count from the (localized) commit
messages by introducing a new file called 'current-fixups' that keeps
track of the current fixup/squash chain. This file contains a bit more
than just the count (it contains a list of "fixup "/"squash
" lines). This is done on purpose, as it will come in handy for
a fix for the bug where `git rebase --skip` on a final fixup/squash will
leave the commit message in limbo.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c | 78 ++---
 sequencer.h |  6 -
 2 files changed, 49 insertions(+), 35 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5e3a50fafc9..d2e6f33023d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -74,13 +74,6 @@ static GIT_PATH_FUNC(rebase_path_message, 
"rebase-merge/message")
  * previous commit and from the first squash/fixup commit are written
  * to it. The commit message for each subsequent squash/fixup commit
  * is appended to the file as it is processed.
- *
- * The first line of the file is of the form
- * # This is a combination of $count commits.
- * where $count is the number of commits whose messages have been
- * written to the file so far (including the initial "pick" commit).
- * Each time that a commit message is processed, this line is read and
- * updated. It is deleted just before the combined commit is made.
  */
 static GIT_PATH_FUNC(rebase_path_squash_msg, "rebase-merge/message-squash")
 /*
@@ -91,6 +84,11 @@ static GIT_PATH_FUNC(rebase_path_squash_msg, 
"rebase-merge/message-squash")
  * commit without opening the editor.)
  */
 static GIT_PATH_FUNC(rebase_path_fixup_msg, "rebase-merge/message-fixup")
+/*
+ * This file contains the list fixup/squash commands that have been
+ * accumulated into message-fixup or message-squash so far.
+ */
+static GIT_PATH_FUNC(rebase_path_current_fixups, "rebase-merge/current-fixups")
 /*
  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
  * GIT_AUTHOR_DATE that will be used for the commit that is currently
@@ -253,6 +251,7 @@ int sequencer_remove_state(struct replay_opts *opts)
for (i = 0; i < opts->xopts_nr; i++)
free(opts->xopts[i]);
free(opts->xopts);
+   strbuf_release(>current_fixups);
 
strbuf_addstr(, get_dir(opts));
remove_dir_recursively(, 0);
@@ -1329,34 +1328,23 @@ static int update_squash_messages(enum todo_command 
command,
struct commit *commit, struct replay_opts *opts)
 {
struct strbuf buf = STRBUF_INIT;
-   int count, res;
+   int res;
const char *message, *body;
 
-   if (file_exists(rebase_path_squash_msg())) {
+   if (opts->current_fixup_count > 0) {
struct strbuf header = STRBUF_INIT;
-   char *eol, *p;
+   char *eol;
 
-   if (strbuf_read_file(, rebase_path_squash_msg(), 2048) <= 0)
+   if (strbuf_read_file(, rebase_path_squash_msg(), 9) <= 0)
return error(_("could not read '%s'"),
rebase_path_squash_msg());
 
-   p = buf.buf + 1;
-   eol = strchrnul(buf.buf, '\n');
-   if (buf.buf[0] != comment_line_char ||
-   (p += strcspn(p, "0123456789\n")) == eol)
-   return error(_("unexpected 1st line of squash message:"
-  "\n\n\t%.*s"),
-(int)(eol - buf.buf), buf.buf);
-   count = strtol(p, NULL, 10);
-
-   if (count < 1)
-   return error(_("invalid 1st line of squash message:\n"
-  "\n\t%.*s"),
-(int)(eol - buf.buf), buf.buf);
+   eol = buf.buf[0] != comment_line_char ?
+   buf.buf : strchrnul(buf.buf, '\n');
 
strbuf_addf(, "%c ", comment_line_char);
-   strbuf_addf(,
-   _("This is a combination of %d commits."), ++count);
+   strbuf_addf(, _("This is a combination of %d commits."),
+   opts->current_fixup_count + 2);
strbuf_splice(, 0, eol - buf.buf, header.buf, header.len);
strbuf_release();
} else {
@@ -1379,10 +1367,8 @@ static int update_squash_messages(enum todo_command 
command,
 rebase_path_fixup_msg());
}
 
-   count = 2;
strbuf_addf(, "%c ", comment_line_char);
-  

[PATCH v4 4/4] rebase --skip: clean up commit message after a failed fixup/squash

[Johannes Schindelin]

During a series of fixup/squash commands, the interactive rebase builds
up a commit message with comments. This will be presented to the user in
the editor if at least one of those commands was a `squash`.

In any case, the commit message will be cleaned up eventually, removing
all those intermediate comments, in the final step of such a
fixup/squash chain.

However, if the last fixup/squash command in such a chain fails with
merge conflicts, and if the user then decides to skip it (or resolve it
to a clean worktree and then continue the rebase), the current code
fails to clean up the commit message.

This commit fixes that behavior.

The fix is quite a bit more involved than meets the eye because it is
not only about the question whether we are `git rebase --skip`ing a
fixup or squash. It is also about removing the skipped fixup/squash's
commit message from the accumulated commit message. And it is also about
the question whether we should let the user edit the final commit
message or not ("Was there a squash in the chain *that was not
skipped*?").

For example, in this case we will want to fix the commit message, but
not open it in an editor:

pick<- succeeds
fixup   <- succeeds
squash  <- fails, will be skipped

This is where the newly-introduced `current-fixups` file comes in real
handy. A quick look and we can determine whether there was a non-skipped
squash. We only need to make sure to keep it up to date with respect to
skipped fixup/squash commands. As a bonus, we can even avoid committing
unnecessarily, e.g. when there was only one fixup, and it failed, and
was skipped.

To fix only the bug where the final commit message was not cleaned up
properly, but without fixing the rest, would have been more complicated
than fixing it all in one go, hence this commit lumps together more than
a single concern.

For the same reason, this commit also adds a bit more to the existing
test case for the regression we just fixed.

The diff is best viewed with --color-moved.

Signed-off-by: Johannes Schindelin 
---
 sequencer.c| 113 -
 t/t3418-rebase-continue.sh |  35 ++--
 2 files changed, 131 insertions(+), 17 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 56166b0d6c7..cec180714ef 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2779,19 +2779,16 @@ static int continue_single_pick(void)
return run_command_v_opt(argv, RUN_GIT_CMD);
 }
 
-static int commit_staged_changes(struct replay_opts *opts)
+static int commit_staged_changes(struct replay_opts *opts,
+struct todo_list *todo_list)
 {
unsigned int flags = ALLOW_EMPTY | EDIT_MSG;
+   unsigned int final_fixup = 0, is_clean;
 
if (has_unstaged_changes(1))
return error(_("cannot rebase: You have unstaged changes."));
-   if (!has_uncommitted_changes(0)) {
-   const char *cherry_pick_head = git_path_cherry_pick_head();
 
-   if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
-   return error(_("could not remove CHERRY_PICK_HEAD"));
-   return 0;
-   }
+   is_clean = !has_uncommitted_changes(0);
 
if (file_exists(rebase_path_amend())) {
struct strbuf rev = STRBUF_INIT;
@@ -2804,19 +2801,107 @@ static int commit_staged_changes(struct replay_opts 
*opts)
if (get_oid_hex(rev.buf, _amend))
return error(_("invalid contents: '%s'"),
rebase_path_amend());
-   if (oidcmp(, _amend))
+   if (!is_clean && oidcmp(, _amend))
return error(_("\nYou have uncommitted changes in your "
   "working tree. Please, commit them\n"
   "first and then run 'git rebase "
   "--continue' again."));
+   /*
+* When skipping a failed fixup/squash, we need to edit the
+* commit message, the current fixup list and count, and if it
+* was the last fixup/squash in the chain, we need to clean up
+* the commit message and if there was a squash, let the user
+* edit it.
+*/
+   if (is_clean && !oidcmp(, _amend) &&
+   opts->current_fixup_count > 0 &&
+   file_exists(rebase_path_stopped_sha())) {
+   const char *p = opts->current_fixups.buf;
+   int len = opts->current_fixups.len;
+
+   opts->current_fixup_count--;
+   if (!len)
+   BUG("Incorrect current_fixups:\n%s", p);
+   while (len && p[len - 1] != '\n')
+   len--;
+   strbuf_setlen(>current_fixups, len);

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Elijah Newren]

On Fri, Apr 27, 2018 at 11:37 AM, Eckhard Maaß
 wrote:
> On Fri, Apr 27, 2018 at 11:23:56AM +0900, Junio C Hamano wrote:
>> I think demoting from copy to rename-only is a good idea, at least
>> for now, because I do not believe we have figured out what we want
>> to happen when we detect copied files are involved in a merge.
>
> Does anyone know some threads concerning that topic? I tried to search
> for it, but somehow "merge copy detection" did not find me useful
> threads so far. I would be interested in that topic anyway, so I would
> like to know what the ideas are that floated so far.
>

I doubt it has ever been discussed before this thread.  But, if you're
curious, I'll try to dump a few thoughts.  Let's say we have branches
A and B, and:
   A: modifies file z
   B: copies z to y

Should the modifications to z done in A propagate to both z and y?  If
not, what good is copy detection?  If so, then there are several
ramifications...

- If B not only copied z but also first modified it, then do we have
  potential conflicts with both z and y -- possibly the exact same
  conflicts, making the user resolve them repeatedly?

- What if A copied z to x?  Do changes to z propagate to all three of
  z and x and y?  Do changes to either x or y affect z?  Do they
  affect each other?

- If A deleted z, does that give us a copy/delete conflict for y?  Do
  we also have to worry about copy/add conflicts?  copy/add/delete?
  rename/copy (multiple variants)?  copy/copy?

- Extra degrees of freedom may mean new conflict types:

  - The extra degrees of freedom from renames introduced multiple new
conflict types (e.g. rename/add, rename/rename(1to2),
rename/rename(2to1)).

  - Directory rename detection added more (rename/rename/rename(1to3),
rename/rename(Nto1), add/add/add, directory/file/file, n-fold
transitive rename, etc.), -- and forced us to specify various
rules to limit the possibility space so that conflicts could be
representable in the index and understandable by the user.

  - I suspect adding copies would add new types of conflicts we
haven't thought of yet.

The more I think about it, the more I think that attempting to detect
copies in a merge algorithm just doesn't make sense.  Anything I can
think of that someone might attempt to use detected copies for would
just surprise users in a bad way...and it'd open up a big can of edge
and corner case problems as well.

Re: [RFC PATCH] checkout: Force matching mtime between files

[Ævar Arnfjörð Bjarmason]

On Fri, Apr 27, 2018 at 7:18 PM, Michał Górny  wrote:
> W dniu śro, 25.04.2018 o godzinie 11∶18 -0400, użytkownik Marc Branchaud
> napisał:
>> On 2018-04-25 04:48 AM, Junio C Hamano wrote:
>> > "Robin H. Johnson"  writes:
>> >
>> > > In the thread from 6 years ago, you asked about tar's behavior for
>> > > mtimes. 'tar xf' restores mtimes from the tar archive, so relative
>> > > ordering after restore would be the same, and would only rebuild if the
>> > > original source happened to be dirty.
>> > >
>> > > This behavior is already non-deterministic in Git, and would be improved
>> > > by the patch.
>> >
>> > But Git is not an archiver (tar), but is a source code control
>> > system, so I do not think we should spend any extra cycles to
>> > "improve" its behaviour wrt the relative ordering, at least for the
>> > default case.  Only those who rely on having build artifact *and*
>> > source should pay the runtime (and preferrably also the
>> > maintainance) cost.
>>
>> Anyone who uses "make" or some other mtime-based tool is affected by
>> this.  I agree that it's not "Everyone" but it sure is a lot of people.
>>
>> Are we all that sure that the performance hit is that drastic?  After
>> all, we've just done write_entry().  Calling utime() at that point
>> should just hit the filesystem cache.
>>
>> > The best approach to do so is to have those people do the "touch"
>> > thing in their own post-checkout hook.  People who use Git as the
>> > source control system won't have to pay runtime cost of doing the
>> > touch thing, and we do not have to maintain such a hook script.
>> > Only those who use the "feature" would.
>>
>> The post-checkout hook approach is not exactly straightforward.
>>
>> Naively, it's simply
>>
>>   for F in `git diff --name-only $1 $2`; do touch "$F"; done
>>
>> But consider:
>>
>> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's
>> proposed patch also doesn't deal with symlinks.)  Let's assume that a
>> hook can be crafted will all possible sophistication.  There are still
>> some fundamental problems:
>>
>> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
>> identical so the above loop does nothing.  Offhand I'm not even sure how
>> a hook might get the right files in this case.
>>
>> * The hook has to be set up in every repo and submodule (at least until
>> something like Ævar's experiments come to fruition).
>>
>> * A fresh clone can't run the hook.  This is especially important when
>> dealing with submodules.  (In one case where we were bit by this, make
>> though that half of a fresh submodule clone's files were stale, and
>> decided to re-autoconf the entire thing.)
>>
>>
>> I just don't think the hook approach can completely solve the problem.
>>
>
> There's also the performance aspect.  If we deal with checkouts that
> include 1000+ files on a busy system (i.e. when mtimes really become
> relevant), calling utime() instantly has a good chance of hitting warm
> cache.  On the other hand, post-checkout hook has a greater risk of
> running cold cache, i.e. writing to all inodes twice.

The FS cache is evicted on a LRU basis. What you're saying is true,
but in the two different implementations there's maybe a 2-3 second
gap between what git is doing and the post-checkout hook is doing. If
the system is under such memory pressure that you've evicted the pages
you just touched you're probably screwed anyway. Maybe I've missed
something here, but this point seems moot.

There's certainly other good arguments against using the current hook
implementation for this, e.g. not being able to do this on clone as
noted upthread.

I think patches that made this configurable in some way in git would
be worth looking at, and due to the subject matter it might make sense
to have it in the core distribution as a non-hook, but I think the
default behavior should always be what it is now, since almost nobody
cares about these edge case,s and users should have to opt-in to use
behavior to work around them.

Re: Fetching tags overwrites existing tags

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 24, 2018 at 9:57 PM, Wink Saville  wrote:
> If have a repository with a tag "v1.0.0" and I add a remote repository
> which also has a tag "v1.0.0" tag is overwritten.

I feel like this thread has gotten somewhat side-tracked by the valid
discussion about whether we should have remote tracking tags, but the
much easier thing to fix is that the "+" prefix for refs/tags/* means
nothing.

I noticed this when working on fetch.pruneTags in the last release,
but didn't dig further.

I.e. if you clone git.git and update "master" and a tag:

$ git fetch origin 'refs/heads/*:refs/heads/*' --dry-run
>From github.com:git/git
 ! [rejected]  master -> master  (non-fast-forward)
$ git fetch origin '+refs/heads/*:refs/heads/*' --dry-run
>From github.com:git/git
 + 969e05fae2...1f1cddd558 master -> master  (forced update)

Here "+" does the right thing, but then:

$ git fetch origin 'refs/tags/*:refs/tags/*' --dry-run
>From github.com:git/git
 t [tag update]v2.17.0-> v2.17.0
$ git fetch origin '+refs/tags/*:refs/tags/*' --dry-run
>From github.com:git/git
 t [tag update]v2.17.0-> v2.17.0

Here the former shouldn't be clobbering the existing tag.

[PATCH v2 7/6] doc: normalize [--options] to [options] in git-diff

[Andreas Heiduk]

SYNOPSIS and other manuals use [options] but DESCRIPTION
used [--options].

Signed-off-by: Andreas Heiduk 
---
 Documentation/git-diff.txt | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 6593b58299..7c2c442700 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -21,7 +21,7 @@ Show changes between the working tree and the index or a 
tree, changes
 between the index and a tree, changes between two trees, changes between
 two blob objects, or changes between two files on disk.
 
-'git diff' [--options] [--] [...]::
+'git diff' [options] [--] [...]::
 
This form is to view the changes you made relative to
the index (staging area for the next commit).  In other
@@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk.
further add to the index but you still haven't.  You can
stage these changes by using linkgit:git-add[1].
 
-'git diff' [--options] --no-index [--]  ::
+'git diff' [options] --no-index [--]  ::
 
This form is to compare the given two paths on the
filesystem.  You can omit the `--no-index` option when
@@ -38,7 +38,7 @@ two blob objects, or changes between two files on disk.
or when running the command outside a working tree
controlled by Git.
 
-'git diff' [--options] --cached [] [--] [...]::
+'git diff' [options] --cached [] [--] [...]::
 
This form is to view the changes you staged for the next
commit relative to the named .  Typically you
@@ -48,7 +48,7 @@ two blob objects, or changes between two files on disk.
 is not given, it shows all staged changes.
--staged is a synonym of --cached.
 
-'git diff' [--options]  [--] [...]::
+'git diff' [options]  [--] [...]::
 
This form is to view the changes you have in your
working tree relative to the named .  You can
@@ -56,18 +56,18 @@ two blob objects, or changes between two files on disk.
branch name to compare with the tip of a different
branch.
 
-'git diff' [--options]   [--] [...]::
+'git diff' [options]   [--] [...]::
 
This is to view the changes between two arbitrary
.
 
-'git diff' [--options] .. [--] [...]::
+'git diff' [options] .. [--] [...]::
 
This is synonymous to the previous form.  If  on
one side is omitted, it will have the same effect as
using HEAD instead.
 
-'git diff' [--options] \... [--] [...]::
+'git diff' [options] \... [--] [...]::
 
This form is to view the changes on the branch containing
and up to the second , starting at a common ancestor
-- 
2.16.2

Re: Fetching tags overwrites existing tags

[Bryan Turner]

On Fri, Apr 27, 2018 at 12:08 PM, Wink Saville  wrote:
>
> The other change was rather than using 
> ""+refs/tags/*:refs/remote-tags/$name/*"
> I've changed it to "+refs/tags/*:refs/remote/tags/$name/*" which seems 
> cleaner.
> Again, if remote-tags is preferred I'll change it back.


>From looking at the code, it looks like you mean
"+refs/tags/*:refs/remotes/tags/$name/*".

The issue with that approach is that it collides with a remote named
"tags". "refs/remote-tags", on the other hand, represents a new-to-Git
path, one that won't already be in use by any other standard
functionality. That seems like a better approach than hoping no one
out there will call one of their remotes "tags".

Bryan

Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis

[Eric Sunshine]

On Fri, Apr 27, 2018 at 2:40 PM, Andreas Heiduk  wrote:
> Am 27.04.2018 um 19:33 schrieb Eric Sunshine:
>> On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk  wrote:
>>> @@ -13,7 +13,7 @@ SYNOPSIS
>>> -'git diff' [options] [--no-index] [--]  
>>> +'git diff' [options] --no-index [--]  
>>> @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk.
>>> -'git diff' --no-index [--options] [--] [...]::
>>> +'git diff' [--options] --no-index [--]  ::
>>
>> Not a problem introduced by this patch, but shouldn't this say
>> "[options]" rather than "[--options]"? Since the aim of this patch
>> series is to clean up botches and normalize documentation, perhaps it
>> could also fix this oddness(?).
>
> Well, in the SYNOPSIS it is always `[options]` for all variants but in
> the DESCRIPTION it is always `[--options]` for all variants. Fixing the
> other variants would stretch the "subject" line of the patch a little
> bit to far ;-)

I wasn't suggesting that this patch should fix that issue (it
shouldn't) but that it could/should be done by a separate new patch
since it's a distinct change. (That's why I was careful to say "aim of
this patch _series_".)

Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis

[Andreas Heiduk]

Am 27.04.2018 um 20:45 schrieb Martin Ågren:
> On 27 April 2018 at 20:28, Andreas Heiduk  wrote:
>> Am 27.04.2018 um 19:18 schrieb Martin Ågren:
>>> On 27 April 2018 at 19:04, Andreas Heiduk  wrote:
 The two '' parameters are not optional but the option
 '--no-index' is. Also move the `--options` part to the same
 place where the other variants show them.
>>>
>>> Should this commit message be updated after the changes you did to
>>> address Junio's comment? This text suggests you want to place --no-index
>>> in [] (and you did in v1) but you do not do that below.
>>>
 All three items are already correct in the synopsis.
>>>
>>> Same here, now you actually do change things there.
>>>
 Signed-off-by: Andreas Heiduk 
 Reviewed-by: Martin Ågren 
>>>
>>> Strictly speaking, my Reviewed-by was on another patch. I do find this
>>
>> Sorry, I've added that trailer after reading "The diff LGTM.", then
>> applied Junio's changes and forgot to remove the trailer.
>>
>>> one better though thanks to Junio's suggestion (except the mismatch with
>>> the commit message).
>>
>> I'll fix that with this:
>>
>> doc: align 'diff --no-index' in text with synopsis
> 
> s/with/and/ since they both change? It's not that the first changes to
> match the second, but they actually both change to match each other (and
> to be correct, obviously).

Corrected

> 
>> Make the two '' parameters in DESCRIPTION mandatory and
>> move the `--options` part to the same place where the other
>> variants show them. And finally make `--no-index` in SYNOPSIS
>> as mandatory as in DESCRIPTION.
> 
> Great! Junio had some good reasoning about how --no-index is
> sometimes optional, but not always. Not sure if it's worth spelling that
> out. (Although one could argue that it already did trip us up once. :-))

The post-context already explains that.

> Eric's point about "--options" vs "options" seemed right to me. If you
> address that, note that this message says "--options".

Re: Fetching tags overwrites existing tags

[Wink Saville]

On Thu, Apr 26, 2018 at 4:24 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>
> Hence (1) we should detect and error out when --prefix-tags is used
> with mirror fetch near where we do the same for track used without
> mirror fetch already, (2) detect and error out when --prefix-tags is
> used with track, and (3) add "+refs/tags/*:refs/remote-tags/$name/*"
> just once without paying attention to track here.  We may not even
> want add_remote_tags() helper function if we go that route.
>

I've replied to the thread using format-email/send-email with the
subject: "[RFC PATCH v2] Teach remote add the --prefix-tags option",
but I misspelled Junio's email address :(

I've tried to address the issues pointed out by Junio. But I've choosen
not to do "(2) detect and error out when --prefix-tags is used with track".
My thinking is tags are independent of tracking and it seems reasonable
that they sould be included if requested. If I'm wrong I'll certainly fix it.

The other change was rather than using ""+refs/tags/*:refs/remote-tags/$name/*"
I've changed it to "+refs/tags/*:refs/remote/tags/$name/*" which seems cleaner.
Again, if remote-tags is preferred I'll change it back.

One other question, I'm not sure "--prefix-tags" is the best name for
the option,
maybe "--sub-tags" or "--nested-tags" or ...

-- Wink

[RFC PATCH v2] Teach remote add the --prefix-tags option

[Wink Saville]

When --prefix-tags is passed to `git remote add` the tagopt is set to
--prefix-tags and a second fetch line is added so tags are placed in
a separate hierarchy per remote.

For example:
  $ git remote add -f --prefix-tags gbenchmark g...@github.com:google/benchmark
  Updating gbenchmark
  warning: no common commits
  remote: Counting objects: 4406, done.
  remote: Compressing objects: 100% (18/18), done.
  remote: Total 4406 (delta 7), reused 13 (delta 6), pack-reused 4382
  Receiving objects: 100% (4406/4406), 1.34 MiB | 7.58 MiB/s, done.
  Resolving deltas: 100% (2865/2865), done.
  From github.com:google/benchmark
   * [new branch]  clangtidy   -> gbenchmark/clangtidy
   * [new branch]  iter_report -> gbenchmark/iter_report
   * [new branch]  master  -> gbenchmark/master
   * [new branch]  releasing   -> gbenchmark/releasing
   * [new branch]  reportercleanup -> gbenchmark/reportercleanup
   * [new branch]  rmheaders   -> gbenchmark/rmheaders
   * [new branch]  v2  -> gbenchmark/v2
   * [new tag] v0.0.9  -> tags/gbenchmark/v0.0.9
   * [new tag] v0.1.0  -> tags/gbenchmark/v0.1.0
   * [new tag] v1.0.0  -> tags/gbenchmark/v1.0.0
   * [new tag] v1.1.0  -> tags/gbenchmark/v1.1.0
   * [new tag] v1.2.0  -> tags/gbenchmark/v1.2.0
   * [new tag] v1.3.0  -> tags/gbenchmark/v1.3.0
   * [new tag] v1.4.0  -> tags/gbenchmark/v1.4.0

And the .git/config remote "gbenchmark" section looks like:
  [remote "gbenchmark"]
url = g...@github.com:google/benchmark
fetch = +refs/heads/*:refs/remotes/gbenchmark/*
fetch = +refs/tags/*:refs/remotes/tags/gbenchmark/*
tagopt = --prefix-tags

Based on a solution proposed by Junio on the email list [1]

[1]: 
https://public-inbox.org/git/xmqqbme51rgn@gitster-ct.c.googlers.com/T/#me7f7f153b8ba742c0dc48d8ec79c280c9682d32e

Helped-by: Junio C Hamano 
Helped-by: Jacob Keller 
Signed-off-by: Wink Saville 
---
 Documentation/git-remote.txt |  8 +--
 builtin/remote.c | 42 
 remote.c |  2 ++
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4feddc0293..c97bf29d46 100644
--- a/Documentation/git-remote.txt
+++ b/Documentation/git-remote.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git remote' [-v | --verbose]
-'git remote add' [-t ] [-m ] [-f] [--[no-]tags] 
[--mirror=]  
+'git remote add' [-t ] [-m ] [-f] [--prefix-tags | --tags | 
--no-tags] [--mirror=]  
 'git remote rename'  
 'git remote remove' 
 'git remote set-head'  (-a | --auto | -d | --delete | )
@@ -54,7 +54,11 @@ With `-f` option, `git fetch ` is run immediately after
 the remote information is set up.
 +
 With `--tags` option, `git fetch ` imports every tag from the
-remote repository.
+remote repository to refs/tags, use --prefix-tags to import them
+to refs/remotes/tags//.
++
+With `--prefix-tags` option, `git fetch ` imports every tag from the
+remote repository to refs/remotes/tags//.
 +
 With `--no-tags` option, `git fetch ` does not import tags from
 the remote repository.
diff --git a/builtin/remote.c b/builtin/remote.c
index 805ffc05cd..39de50bdd6 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -11,7 +11,7 @@
 
 static const char * const builtin_remote_usage[] = {
N_("git remote [-v | --verbose]"),
-   N_("git remote add [-t ] [-m ] [-f] [--tags | 
--no-tags] [--mirror=]  "),
+   N_("git remote add [-t ] [-m ] [-f] [--prefix-tags | 
--tags | --no-tags] [--mirror=]  "),
N_("git remote rename  "),
N_("git remote remove "),
N_("git remote set-head  (-a | --auto | -d | --delete | 
)"),
@@ -101,7 +101,8 @@ static int fetch_remote(const char *name)
 enum {
TAGS_UNSET = 0,
TAGS_DEFAULT = 1,
-   TAGS_SET = 2
+   TAGS_SET = 2,
+   TAGS_SET_PREFIX = 3
 };
 
 #define MIRROR_NONE 0
@@ -123,6 +124,14 @@ static void add_branch(const char *key, const char 
*branchname,
git_config_set_multivar(key, tmp->buf, "^$", 0);
 }
 
+static void add_remote_tags(const char *key, const char *remotename,
+   struct strbuf *tmp)
+{
+   strbuf_reset(tmp);
+   strbuf_addf(tmp, "+refs/tags/*:refs/remotes/tags/%s/*", remotename);
+   git_config_set_multivar(key, tmp->buf, "^$", 0);
+}
+
 static const char mirror_advice[] =
 N_("--mirror is dangerous and deprecated; please\n"
"\t use --mirror=fetch or --mirror=push instead");
@@ -161,6 +170,9 @@ static int add(int argc, const char **argv)
OPT_SET_INT(0, "tags", _tags,
N_("import all tags and associated objects when 
fetching"),
TAGS_SET),
+   

Re: [PATCH v2 1/6] doc: improve formatting in githooks.txt

[Martin Ågren]

On 27 April 2018 at 19:04, Andreas Heiduk  wrote:
> Typeset commands and similar things with as `git foo` instead of
> 'git foo' or 'git-foo' and add linkgit to the commands which run
> the hooks.
>
> Signed-off-by: Andreas Heiduk 
> Reviewed-by: Martin Ågren 

Indeed. The difference between last time (the original patch and the two
fixups) and this patch is precisely the small tweaks that I suggested.

Thanks
Martin

Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis

[Martin Ågren]

On 27 April 2018 at 20:28, Andreas Heiduk  wrote:
> Am 27.04.2018 um 19:18 schrieb Martin Ågren:
>> On 27 April 2018 at 19:04, Andreas Heiduk  wrote:
>>> The two '' parameters are not optional but the option
>>> '--no-index' is. Also move the `--options` part to the same
>>> place where the other variants show them.
>>
>> Should this commit message be updated after the changes you did to
>> address Junio's comment? This text suggests you want to place --no-index
>> in [] (and you did in v1) but you do not do that below.
>>
>>> All three items are already correct in the synopsis.
>>
>> Same here, now you actually do change things there.
>>
>>> Signed-off-by: Andreas Heiduk 
>>> Reviewed-by: Martin Ågren 
>>
>> Strictly speaking, my Reviewed-by was on another patch. I do find this
>
> Sorry, I've added that trailer after reading "The diff LGTM.", then
> applied Junio's changes and forgot to remove the trailer.
>
>> one better though thanks to Junio's suggestion (except the mismatch with
>> the commit message).
>
> I'll fix that with this:
>
> doc: align 'diff --no-index' in text with synopsis

s/with/and/ since they both change? It's not that the first changes to
match the second, but they actually both change to match each other (and
to be correct, obviously).

> Make the two '' parameters in DESCRIPTION mandatory and
> move the `--options` part to the same place where the other
> variants show them. And finally make `--no-index` in SYNOPSIS
> as mandatory as in DESCRIPTION.

Great! Junio had some good reasoning about how --no-index is
sometimes optional, but not always. Not sure if it's worth spelling that
out. (Although one could argue that it already did trip us up once. :-))

Eric's point about "--options" vs "options" seemed right to me. If you
address that, note that this message says "--options".

Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis

[Martin Ågren]

On 27 April 2018 at 20:40, Andreas Heiduk  wrote:
> Am 27.04.2018 um 19:33 schrieb Eric Sunshine:
>> On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk  wrote:
>>> The two '' parameters are not optional but the option
>>> '--no-index' is. Also move the `--options` part to the same
>>> place where the other variants show them.
>>>
>>> All three items are already correct in the synopsis.
>>>
>>> Signed-off-by: Andreas Heiduk 
>>> ---
>>> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
>>> @@ -13,7 +13,7 @@ SYNOPSIS
>>> -'git diff' [options] [--no-index] [--]  
>>> +'git diff' [options] --no-index [--]  
>>> @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk.
>>> -'git diff' --no-index [--options] [--] [...]::
>>> +'git diff' [--options] --no-index [--]  ::
>>
>> Not a problem introduced by this patch, but shouldn't this say
>> "[options]" rather than "[--options]"? Since the aim of this patch
>> series is to clean up botches and normalize documentation, perhaps it
>> could also fix this oddness(?).
>>
>
> Well, in the SYNOPSIS it is always `[options]` for all variants but in
> the DESCRIPTION it is always `[--options]` for all variants. Fixing the
> other variants would stretch the "subject" line of the patch a little
> bit to far ;-)

Hmm, I do not think it's always though. It's pretty consistent in its
inconsistency, but "git diff [options]  " goes the other
way. Maybe that's patch 1/7...

Re: [PATCH v2 6/6] doc: add note about shell quoting to revision.txt

[Andreas Heiduk]

Am 27.04.2018 um 19:36 schrieb Eric Sunshine:
> On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk  wrote:
>> Signed-off-by: Andreas Heiduk 
>> Reviewed-by: Junio C Hamano 
>> ---
>> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
>> @@ -186,6 +190,8 @@ existing tag object.
>> +  Depending on the given text the shell's word splitting rules might
>> +  require additional quoting.
> 
> s/text/&,/
> 

Fixed, Thanks

Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis

[Andreas Heiduk]

Am 27.04.2018 um 19:33 schrieb Eric Sunshine:
> On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk  wrote:
>> The two '' parameters are not optional but the option
>> '--no-index' is. Also move the `--options` part to the same
>> place where the other variants show them.
>>
>> All three items are already correct in the synopsis.
>>
>> Signed-off-by: Andreas Heiduk 
>> ---
>> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
>> @@ -13,7 +13,7 @@ SYNOPSIS
>> -'git diff' [options] [--no-index] [--]  
>> +'git diff' [options] --no-index [--]  
>> @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk.
>> -'git diff' --no-index [--options] [--] [...]::
>> +'git diff' [--options] --no-index [--]  ::
> 
> Not a problem introduced by this patch, but shouldn't this say
> "[options]" rather than "[--options]"? Since the aim of this patch
> series is to clean up botches and normalize documentation, perhaps it
> could also fix this oddness(?).
> 

Well, in the SYNOPSIS it is always `[options]` for all variants but in
the DESCRIPTION it is always `[--options]` for all variants. Fixing the
other variants would stretch the "subject" line of the patch a little
bit to far ;-)

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Eckhard Maaß]

On Fri, Apr 27, 2018 at 11:23:56AM +0900, Junio C Hamano wrote:
> I think demoting from copy to rename-only is a good idea, at least
> for now, because I do not believe we have figured out what we want
> to happen when we detect copied files are involved in a merge.

Does anyone know some threads concerning that topic? I tried to search
for it, but somehow "merge copy detection" did not find me useful
threads so far. I would be interested in that topic anyway, so I would
like to know what the ideas are that floated so far.

Greetings,
Eckhard

[no subject]

[Elijah Newren]

From: Elijah Newren 

On Thu, Apr 26, 2018 at 5:54 PM, Ben Peart  wrote:

> Can you write the documentation that clearly explains the exact behavior you
> want?  That would kill two birds with one stone... :)

Sure, something like the following is what I envision, and I've tried to
include the suggestion from Junio to document the copy behavior in the
merge-recursive documentation.

-- 8< --
Subject: [PATCH] fixup! merge: Add merge.renames config setting

---
 Documentation/merge-config.txt | 3 +--
 Documentation/merge-strategies.txt | 5 +++--
 merge-recursive.c  | 8 
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/merge-config.txt b/Documentation/merge-config.txt
index 59848e5634..662c2713ca 100644
--- a/Documentation/merge-config.txt
+++ b/Documentation/merge-config.txt
@@ -41,8 +41,7 @@ merge.renameLimit::
 merge.renames::
Whether and how Git detects renames.  If set to "false",
rename detection is disabled. If set to "true", basic rename
-   detection is enabled.  If set to "copies" or "copy", Git will
-   detect copies, as well.  Defaults to the value of diff.renames.
+   detection is enabled.  Defaults to the value of diff.renames.
 
 merge.renormalize::
Tell Git that canonical representation of files in the
diff --git a/Documentation/merge-strategies.txt 
b/Documentation/merge-strategies.txt
index 1e0728aa12..aa66cbe41e 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -23,8 +23,9 @@ recursive::
causing mismerges by tests done on actual merge commits
taken from Linux 2.6 kernel development history.
Additionally this can detect and handle merges involving
-   renames.  This is the default merge strategy when
-   pulling or merging one branch.
+   renames, but currently cannot make use of detected
+   copies.  This is the default merge strategy when pulling
+   or merging one branch.
 +
 The 'recursive' strategy can take the following options:
 
diff --git a/merge-recursive.c b/merge-recursive.c
index 6cc4404144..b618f134d2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -564,6 +564,14 @@ static struct string_list *get_renames(struct 
merge_options *o,
opts.flags.recursive = 1;
opts.flags.rename_empty = 0;
opts.detect_rename = merge_detect_rename(o);
+   /*
+* We do not have logic to handle the detection of copies.  In
+* fact, it may not even make sense to add such logic: would we
+* really want a change to a base file to be propagated through
+* multiple other files by a merge?
+*/
+   if (opts.detect_rename > DIFF_DETECT_RENAME)
+   opts.detect_rename = DIFF_DETECT_RENAME;
opts.rename_limit = o->merge_rename_limit >= 0 ? o->merge_rename_limit :
o->diff_rename_limit >= 0 ? o->diff_rename_limit :
1000;
-- 
2.17.0.294.g8e3b83c0e3

Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis

[Andreas Heiduk]

Am 27.04.2018 um 19:18 schrieb Martin Ågren:
> On 27 April 2018 at 19:04, Andreas Heiduk  wrote:
>> The two '' parameters are not optional but the option
>> '--no-index' is. Also move the `--options` part to the same
>> place where the other variants show them.
> 
> Should this commit message be updated after the changes you did to
> address Junio's comment? This text suggests you want to place --no-index
> in [] (and you did in v1) but you do not do that below.
> 
>> All three items are already correct in the synopsis.
> 
> Same here, now you actually do change things there.
> 
>> Signed-off-by: Andreas Heiduk 
>> Reviewed-by: Martin Ågren 
> 
> Strictly speaking, my Reviewed-by was on another patch. I do find this

Sorry, I've added that trailer after reading "The diff LGTM.", then
applied Junio's changes and forgot to remove the trailer.

> one better though thanks to Junio's suggestion (except the mismatch with
> the commit message).

I'll fix that with this:

doc: align 'diff --no-index' in text with synopsis

Make the two '' parameters in DESCRIPTION mandatory and
move the `--options` part to the same place where the other
variants show them. And finally make `--no-index` in SYNOPSIS
as mandatory as in DESCRIPTION.

Confusing documentation for git apply -p

[kelly elton]

https://git-scm.com/docs/git-apply#git-apply--pltngt

>  -p
> Remove  leading slashes from traditional diff paths. The default is 1.

This suggests to me the following outcomes:
1) home/user/repos/myrepo with -p1 becomes home/user/repos/myrepo
2) home/user/repos/myrepo with -p2 becomes home/user/repos/myrepo
3) /home/user/repos/myrepo with -p1 becomes home/user/repos/myrepo
4) /home/user/repos/myrepo with -p2 becomes home/user/repos/myrepo
5) //home/user/repos/myrepo with -p1 becomes /home/user/repos/myrepo
6) //home/user/repos/myrepo with -p2 becomes home/user/repos/myrepo

`Remove  leading slashes`...That's not really what's happening.

What seems to actually happen is that it is removing directories from the path:
1) home/user/repos/myrepo with -p1 becomes home/user/repos/myrepo
2) home/user/repos/myrepo with -p2 becomes user/repos/myrepo

This argument seems to be removing folders from the path, not slashes.

I'm sure it's doing both removing slashes and folders, but at
different times, which makes it even more confusing to anyone trying
to wrap their head around it.

-p1= /home/a -> home/a
-p1= home/a -> home/a ?? Does this happen, or does it actually remove `home`???
-p2=/home/a -> home/a OR a ??? I have no idea after reading the docs
and seeing different behavior. I literally have to experiment with it
to know what it does in each scenario.
-p2=home/a -> a ?? folder and slash removed.

All of this doesn't have to be so complicated, it's just being
expressed in a complicated way.

Break this out into two separate functions maybe, or update the
documentation so it's clearer how this behaves.

> -p - number of folders in path to remove from the beginning of the path.

Not great, but clearer than what's there.


Thanks,
Kelly Elton
http://www.kellyelton.com

Re: [PATCH v2 6/6] doc: add note about shell quoting to revision.txt

[Eric Sunshine]

On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk  wrote:
> Signed-off-by: Andreas Heiduk 
> Reviewed-by: Junio C Hamano 
> ---
> diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
> @@ -186,6 +190,8 @@ existing tag object.
> +  Depending on the given text the shell's word splitting rules might
> +  require additional quoting.

s/text/&,/

Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis

[Eric Sunshine]

On Fri, Apr 27, 2018 at 1:04 PM, Andreas Heiduk  wrote:
> The two '' parameters are not optional but the option
> '--no-index' is. Also move the `--options` part to the same
> place where the other variants show them.
>
> All three items are already correct in the synopsis.
>
> Signed-off-by: Andreas Heiduk 
> ---
> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
> -'git diff' [options] [--no-index] [--]  
> +'git diff' [options] --no-index [--]  
> @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk.
> -'git diff' --no-index [--options] [--] [...]::
> +'git diff' [--options] --no-index [--]  ::

Not a problem introduced by this patch, but shouldn't this say
"[options]" rather than "[--options]"? Since the aim of this patch
series is to clean up botches and normalize documentation, perhaps it
could also fix this oddness(?).

Missing --relative documentation

[kelly elton]

git format-patch is missing documentation for --relative.
There is also no auto complete(or tab complete, whatever it's called)
for the --relative switch/argument.

https://stackoverflow.com/a/16309416/222054

Thanks,
Kelly Elton
http://www.kellyelton.com

Re: [RFC PATCH] checkout: Force matching mtime between files

[Michał Górny]

W dniu śro, 25.04.2018 o godzinie 11∶18 -0400, użytkownik Marc Branchaud
napisał:
> On 2018-04-25 04:48 AM, Junio C Hamano wrote:
> > "Robin H. Johnson"  writes:
> > 
> > > In the thread from 6 years ago, you asked about tar's behavior for
> > > mtimes. 'tar xf' restores mtimes from the tar archive, so relative
> > > ordering after restore would be the same, and would only rebuild if the
> > > original source happened to be dirty.
> > > 
> > > This behavior is already non-deterministic in Git, and would be improved
> > > by the patch.
> > 
> > But Git is not an archiver (tar), but is a source code control
> > system, so I do not think we should spend any extra cycles to
> > "improve" its behaviour wrt the relative ordering, at least for the
> > default case.  Only those who rely on having build artifact *and*
> > source should pay the runtime (and preferrably also the
> > maintainance) cost.
> 
> Anyone who uses "make" or some other mtime-based tool is affected by 
> this.  I agree that it's not "Everyone" but it sure is a lot of people.
> 
> Are we all that sure that the performance hit is that drastic?  After 
> all, we've just done write_entry().  Calling utime() at that point 
> should just hit the filesystem cache.
> 
> > The best approach to do so is to have those people do the "touch"
> > thing in their own post-checkout hook.  People who use Git as the
> > source control system won't have to pay runtime cost of doing the
> > touch thing, and we do not have to maintain such a hook script.
> > Only those who use the "feature" would.
> 
> The post-checkout hook approach is not exactly straightforward.
> 
> Naively, it's simply
> 
>   for F in `git diff --name-only $1 $2`; do touch "$F"; done
> 
> But consider:
> 
> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's 
> proposed patch also doesn't deal with symlinks.)  Let's assume that a 
> hook can be crafted will all possible sophistication.  There are still 
> some fundamental problems:
> 
> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are 
> identical so the above loop does nothing.  Offhand I'm not even sure how 
> a hook might get the right files in this case.
> 
> * The hook has to be set up in every repo and submodule (at least until 
> something like Ævar's experiments come to fruition).
> 
> * A fresh clone can't run the hook.  This is especially important when 
> dealing with submodules.  (In one case where we were bit by this, make 
> though that half of a fresh submodule clone's files were stale, and 
> decided to re-autoconf the entire thing.)
> 
> 
> I just don't think the hook approach can completely solve the problem.
> 

There's also the performance aspect.  If we deal with checkouts that
include 1000+ files on a busy system (i.e. when mtimes really become
relevant), calling utime() instantly has a good chance of hitting warm
cache.  On the other hand, post-checkout hook has a greater risk of
running cold cache, i.e. writing to all inodes twice.

-- 
Best regards,
Michał Górny

Re: [PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis

[Martin Ågren]

On 27 April 2018 at 19:04, Andreas Heiduk  wrote:
> The two '' parameters are not optional but the option
> '--no-index' is. Also move the `--options` part to the same
> place where the other variants show them.

Should this commit message be updated after the changes you did to
address Junio's comment? This text suggests you want to place --no-index
in [] (and you did in v1) but you do not do that below.

> All three items are already correct in the synopsis.

Same here, now you actually do change things there.

> Signed-off-by: Andreas Heiduk 
> Reviewed-by: Martin Ågren 

Strictly speaking, my Reviewed-by was on another patch. I do find this
one better though thanks to Junio's suggestion (except the mismatch with
the commit message).

Thanks for continuing with this series.

Martin

> ---
>  Documentation/git-diff.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
> index b0c1bb95c8..6593b58299 100644
> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
>  'git diff' [options] --cached [] [--] [...]
>  'git diff' [options]   [--] [...]
>  'git diff' [options]  
> -'git diff' [options] [--no-index] [--]  
> +'git diff' [options] --no-index [--]  
>
>  DESCRIPTION
>  ---
> @@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk.
> further add to the index but you still haven't.  You can
> stage these changes by using linkgit:git-add[1].
>
> -'git diff' --no-index [--options] [--] [...]::
> +'git diff' [--options] --no-index [--]  ::
>
> This form is to compare the given two paths on the
> filesystem.  You can omit the `--no-index` option when

Re: In some rebases, `exec git -C ...` has wrong working directory

[William Chargin]

Hi Johannes,

Thanks for your reply.

Part of my confusion was regarding the interaction between `-C` and
`--git-dir`. For instance, we have

$ git --git-dir target -C /tmp/tmp.Cl4aXMSVis init
Initialized empty Git repository in /tmp/tmp.Cl4aXMSVis/target/

which makes sense and is what I expected: the `-C` and `--git-dir`
values are joined, as suggested by the docs for `-C` in git(1). But with

$ git --git-dir /tmp/tmp.Cl4aXMSVis/repo -C /tmp/tmp.Cl4aXMSVis init
Initialized empty Git repository in /tmp/tmp.Cl4aXMSVis/repo/

it appears that the `-C` argument is ignored entirely. Is this because
running `git -C foo ...` is equivalent to running `cd foo; git ...` in a
shell, so when the `--git-dir` is an absolute path the value of `-C` has
no effect? (Assuming that `GIT_WORK_TREE` is unset.)

In your example:

>exec git -C /somewhere/else show HEAD:some-file >some-other-file

isn't the behavior to copy the version of `some-file` in the repository
being rebased to `some-other-file` in the current working directory,
such that the `-C` has no effect, because the shell redirect is not
affected by the `-C`? (This is what happens for me.) If so, why include
the `-C` in the command?

> I do not think that we can sensibly *remove* GIT_DIR from the environment
> variables passed to the exec'ed command, as we have been doing that for
> ages, and some scripts (as demonstrated above) started relying on that
> behavior. So if we changed it, we would break backwards-compatibility,
> which is something we try to avoid very much in Git.

This makes sense; understood and agreed.

Do you know why `rebase --root` does not set `GIT_DIR`?

> Maybe you could a contribute a patch to the documentation?

Sure; I'd be happy to do that once I understand this a bit better. :-)

Best,
WC

[PATCH v2 4/6] doc: add '-d' and '-o' for 'git push'

[Andreas Heiduk]

Add the missing `-o` shortcut for `--push-option` to the synopsis.
Add the missing `-d` shortcut for `--delete` in the main section.

Signed-off-by: Andreas Heiduk 
Reviewed-by: Martin Ågren 
---
 Documentation/git-push.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 5b08302fc2..f2bbda6e32 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | 
--dry-run] [--receive-pack=]
   [--repo=] [-f | --force] [-d | --delete] [--prune] [-v | 
--verbose]
-  [-u | --set-upstream] [--push-option=]
+  [-u | --set-upstream] [-o  | --push-option=]
   [--[no-]signed|--signed=(true|false|if-asked)]
   [--force-with-lease[=[:]]]
   [--no-verify] [ [...]]
@@ -123,6 +123,7 @@ already exists on the remote side.
will be tab-separated and sent to stdout instead of stderr.  The full
symbolic names of the refs will be given.
 
+-d::
 --delete::
All listed refs are deleted from the remote repository. This is
the same as prefixing all refs with a colon.
-- 
2.16.2

[PATCH v2 5/6] git-svn: remove ''--add-author-from' for 'commit-diff'

[Andreas Heiduk]

The subcommand 'commit-diff' does not support the option
'--add-author-from'.

Signed-off-by: Andreas Heiduk 
Signed-off-by: Eric Wong 
---
 Documentation/git-svn.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt
index d59379ee23..e9615951d2 100644
--- a/Documentation/git-svn.txt
+++ b/Documentation/git-svn.txt
@@ -707,7 +707,7 @@ creating the branch or tag.
 config key: svn.useLogAuthor
 
 --add-author-from::
-   When committing to svn from Git (as part of 'commit-diff', 'set-tree' 
or 'dcommit'
+   When committing to svn from Git (as part of 'set-tree' or 'dcommit'
operations), if the existing log message doesn't already have a
`From:` or `Signed-off-by:` line, append a `From:` line based on the
Git commit's author string.  If you use this, then `--use-log-author`
-- 
2.16.2

[PATCH v2 3/6] doc: clarify ignore rules for git ls-files

[Andreas Heiduk]

Explain that `git ls-files --ignored` requires at least one
of the `--exclude*` options to do its job.

Signed-off-by: Andreas Heiduk 
---
 Documentation/git-ls-files.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 3ac3e3a77d..f3474b2ede 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -53,7 +53,8 @@ OPTIONS
Show only ignored files in the output. When showing files in the
index, print only those matched by an exclude pattern. When
showing "other" files, show only those matched by an exclude
-   pattern.
+   pattern. Standard ignore rules are not automatically activated,
+   therefore at least one of the `--exclude*` options is required.
 
 -s::
 --stage::
-- 
2.16.2

[PATCH v2 6/6] doc: add note about shell quoting to revision.txt

[Andreas Heiduk]

Signed-off-by: Andreas Heiduk 
Reviewed-by: Junio C Hamano 
---
 Documentation/revisions.txt | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/revisions.txt b/Documentation/revisions.txt
index dfcc49c72c..c1d3a40a90 100644
--- a/Documentation/revisions.txt
+++ b/Documentation/revisions.txt
@@ -7,6 +7,10 @@ syntax.  Here are various ways to spell object names.  The
 ones listed near the end of this list name trees and
 blobs contained in a commit.
 
+NOTE: This document shows the "raw" syntax as seen by git. The shell
+and other UIs might require additional quoting to protect special
+characters and to avoid word splitting.
+
 '', e.g. 'dae86e1950b1277e545cee180551750029cfe735', 'dae86e'::
   The full SHA-1 object name (40-byte hexadecimal string), or
   a leading substring that is unique within the repository.
@@ -186,6 +190,8 @@ existing tag object.
   is matched. ':/!-foo' performs a negative match, while ':/!!foo' matches a
   literal '!' character, followed by 'foo'. Any other sequence beginning with
   ':/!' is reserved for now.
+  Depending on the given text the shell's word splitting rules might
+  require additional quoting.
 
 ':', e.g. 'HEAD:README', ':README', 'master:./README'::
   A suffix ':' followed by a path names the blob or tree
-- 
2.16.2

[PATCH v2 1/6] doc: improve formatting in githooks.txt

[Andreas Heiduk]

Typeset commands and similar things with as `git foo` instead of
'git foo' or 'git-foo' and add linkgit to the commands which run
the hooks.

Signed-off-by: Andreas Heiduk 
Reviewed-by: Martin Ågren 
---
 Documentation/githooks.txt | 115 +++--
 1 file changed, 58 insertions(+), 57 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index f877f7b7cd..e3c283a174 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -31,7 +31,7 @@ Hooks can get their arguments via the environment, 
command-line
 arguments, and stdin. See the documentation for each hook below for
 details.
 
-'git init' may copy hooks to the new repository, depending on its
+`git init` may copy hooks to the new repository, depending on its
 configuration. See the "TEMPLATE DIRECTORY" section in
 linkgit:git-init[1] for details. When the rest of this document refers
 to "default hooks" it's talking about the default template shipped
@@ -45,9 +45,9 @@ HOOKS
 applypatch-msg
 ~~
 
-This hook is invoked by 'git am'.  It takes a single
+This hook is invoked by linkgit:git-am[1].  It takes a single
 parameter, the name of the file that holds the proposed commit
-log message.  Exiting with a non-zero status causes 'git am' to abort
+log message.  Exiting with a non-zero status causes `git am` to abort
 before applying the patch.
 
 The hook is allowed to edit the message file in place, and can
@@ -61,7 +61,7 @@ The default 'applypatch-msg' hook, when enabled, runs the
 pre-applypatch
 ~~
 
-This hook is invoked by 'git am'.  It takes no parameter, and is
+This hook is invoked by linkgit:git-am[1].  It takes no parameter, and is
 invoked after the patch is applied, but before a commit is made.
 
 If it exits with non-zero status, then the working tree will not be
@@ -76,33 +76,33 @@ The default 'pre-applypatch' hook, when enabled, runs the
 post-applypatch
 ~~~
 
-This hook is invoked by 'git am'.  It takes no parameter,
+This hook is invoked by linkgit:git-am[1].  It takes no parameter,
 and is invoked after the patch is applied and a commit is made.
 
 This hook is meant primarily for notification, and cannot affect
-the outcome of 'git am'.
+the outcome of `git am`.
 
 pre-commit
 ~~
 
-This hook is invoked by 'git commit', and can be bypassed
+This hook is invoked by linkgit:git-commit[1], and can be bypassed
 with the `--no-verify` option.  It takes no parameters, and is
 invoked before obtaining the proposed commit log message and
 making a commit.  Exiting with a non-zero status from this script
-causes the 'git commit' command to abort before creating a commit.
+causes the `git commit` command to abort before creating a commit.
 
 The default 'pre-commit' hook, when enabled, catches introduction
 of lines with trailing whitespaces and aborts the commit when
 such a line is found.
 
-All the 'git commit' hooks are invoked with the environment
+All the `git commit` hooks are invoked with the environment
 variable `GIT_EDITOR=:` if the command will not bring up an editor
 to modify the commit message.
 
 prepare-commit-msg
 ~~
 
-This hook is invoked by 'git commit' right after preparing the
+This hook is invoked by linkgit:git-commit[1] right after preparing the
 default log message, and before the editor is started.
 
 It takes one to three parameters.  The first is the name of the file
@@ -114,7 +114,7 @@ commit is a merge or a `.git/MERGE_MSG` file exists); 
`squash`
 (if a `.git/SQUASH_MSG` file exists); or `commit`, followed by
 a commit SHA-1 (if a `-c`, `-C` or `--amend` option was given).
 
-If the exit status is non-zero, 'git commit' will abort.
+If the exit status is non-zero, `git commit` will abort.
 
 The purpose of the hook is to edit the message file in place, and
 it is not suppressed by the `--no-verify` option.  A non-zero exit
@@ -127,7 +127,7 @@ help message found in the commented portion of the commit 
template.
 commit-msg
 ~~
 
-This hook is invoked by 'git commit' and 'git merge', and can be
+This hook is invoked by linkgit:git-commit[1] and linkgit:git-merge[1], and 
can be
 bypassed with the `--no-verify` option.  It takes a single parameter,
 the name of the file that holds the proposed commit log message.
 Exiting with a non-zero status causes the command to abort.
@@ -143,16 +143,16 @@ The default 'commit-msg' hook, when enabled, detects 
duplicate
 post-commit
 ~~~
 
-This hook is invoked by 'git commit'. It takes no parameters, and is
+This hook is invoked by linkgit:git-commit[1]. It takes no parameters, and is
 invoked after a commit is made.
 
 This hook is meant primarily for notification, and cannot affect
-the outcome of 'git commit'.
+the outcome of `git commit`.
 
 pre-rebase
 ~~
 
-This hook is called by 'git rebase' and can be used to prevent a
+This hook is called by 

[PATCH v2 2/6] doc: align 'diff --no-index' in text with synopsis

[Andreas Heiduk]

The two '' parameters are not optional but the option
'--no-index' is. Also move the `--options` part to the same
place where the other variants show them.

All three items are already correct in the synopsis.

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

diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index b0c1bb95c8..6593b58299 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -13,7 +13,7 @@ SYNOPSIS
 'git diff' [options] --cached [] [--] [...]
 'git diff' [options]   [--] [...]
 'git diff' [options]  
-'git diff' [options] [--no-index] [--]  
+'git diff' [options] --no-index [--]  
 
 DESCRIPTION
 ---
@@ -29,7 +29,7 @@ two blob objects, or changes between two files on disk.
further add to the index but you still haven't.  You can
stage these changes by using linkgit:git-add[1].
 
-'git diff' --no-index [--options] [--] [...]::
+'git diff' [--options] --no-index [--]  ::
 
This form is to compare the given two paths on the
filesystem.  You can omit the `--no-index` option when
-- 
2.16.2

[PATCH v2 0/6] Some doc-fixes

[Andreas Heiduk]

This reroll incorporates the comments of the first version, including a
"large scale" rewrtie of githooks.txt. I'v added "Reviewed-by" and
"Signed-off-by" as appropriate.

Andreas Heiduk (6):
  doc: improve formatting in githooks.txt
  doc: align 'diff --no-index' in text with synopsis
  doc: clarify ignore rules for git ls-files
  doc: add '-d' and '-o' for 'git push'
  git-svn: remove ''--add-author-from' for 'commit-diff'
  doc: add note about shell quoting to revision.txt

 Documentation/git-diff.txt |   4 +-
 Documentation/git-ls-files.txt |   3 +-
 Documentation/git-push.txt |   3 +-
 Documentation/git-svn.txt  |   2 +-
 Documentation/githooks.txt | 115 +
 Documentation/revisions.txt|   6 +++
 6 files changed, 71 insertions(+), 62 deletions(-)

-- 
2.16.2

Re: [RFC PATCH] checkout: Force matching mtime between files

[Duy Nguyen]

On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud  wrote:
>> The best approach to do so is to have those people do the "touch"
>> thing in their own post-checkout hook.  People who use Git as the
>> source control system won't have to pay runtime cost of doing the
>> touch thing, and we do not have to maintain such a hook script.
>> Only those who use the "feature" would.
>
>
> The post-checkout hook approach is not exactly straightforward.

I am revisiting this because I'm not even happy with my
post-checkout-modified hook suggestion, so..

>
> Naively, it's simply
>
> for F in `git diff --name-only $1 $2`; do touch "$F"; done
>
> But consider:
>
> * Symlinks can cause the wrong file to be touched.  (Granted, Michał's
> proposed patch also doesn't deal with symlinks.)  Let's assume that a hook
> can be crafted will all possible sophistication.  There are still some
> fundamental problems:

OK so this one could be tricky to get right, but it's possible.

>
> * In a "file checkout" ("git checkout -- path/to/file"), $1 and $2 are
> identical so the above loop does nothing.  Offhand I'm not even sure how a
> hook might get the right files in this case.

This is a limitation of the current post-checkout hook. $3==0 from the
hook lets us know this is not a branch switch, but it does not really
tell you the affected paths. If it somehow passes all the given
pathspec to you, then you should be able to do "git ls-files --
$pathspec" which gives you the exact same set of paths that
git-checkout updates. We could do this by setting $4 to "--" and put
all the pathspecs in $5+ [1] e.g. "HEAD@{1} HEAD 0 -- path/to/file" in
the above example.

There is  third case here, if you do "git checkout  --
path/to/file" then it cannot be covered by the current design. I guess
we could set $3 to '2' (retrieve from a tree) to indicate this in
addition to 0 (from index) and 1 (from switching branch) and then $1
could be the tree in question (pathspecs are passed the same way
above)

[1] I wonder if we could have a more generic approach to pass
pathspecs via environment, which could work for more than just this
one hook. Not sure if it's a good idea though.

> * The hook has to be set up in every repo and submodule (at least until
> something like Ævar's experiments come to fruition).

Either --template or core.hooksPath would solve this, or I'll try to
get my "hooks.* config" patch in. I think that one is a good thing to
do anyway because it allows more flexible hook management (and it
could allow multiple hooks of the same name too). With this, we could
avoid adding more command-specific config like core.fsmonitor or
uploadpack.packObjectsHook which to me are hooks.

Another option is extend core.hooksPath for searching multiple places
instead of just one like AEvar suggested.

> * A fresh clone can't run the hook.  This is especially important when
> dealing with submodules.  (In one case where we were bit by this, make
> though that half of a fresh submodule clone's files were stale, and decided
> to re-autoconf the entire thing.)

Both --template and config-based hooks should let you handle this case.

So, I think if we improve the hook system to give more information
(which is something we definitely should do), we could do this without
adding special cases in git. I'm not saying that we should never add
special cases, but at least this lets us play with different kinds of
post-checkout activities before we decide which one would be best
implemented in git.
-- 
Duy

Re: Great Investment Offer

[Gagum Melvin Sikze Kakha]

Hello

In my search for a business partner i got your contact in google 
search. My client is willing to invest $10 Million to $500 
million but my client said he need a trusted partner who he can 
have a meeting at the point of releasing his funds. 

I told my client that you have a good profile with your company 
which i got details about you on my search on google lookup. Can 
we trust you. 

Can we make a plan for a long term business relationship.

Please reply. 

Regards,
Gagum Melvin Sikze Kakha
Tel: 703197576

Re: [PATCH 5/6] git-svn: commit-diff does not support --add-author-from

[Andreas Heiduk]

Am 17.04.2018 um 08:18 schrieb Eric Wong:
> Andreas Heiduk  wrote:
>> Signed-off-by: Andreas Heiduk 
> 
> Thanks.
> Signed-off-by: Eric Wong 
> 
> And pushed for Junio:
[...]

I'd like to keep the patches together, so I borrow your 'Signed-off-By'
from the commit and do a complete reroll of this mini-series.

Re: fixup! [PATCH 1/6] doc: fix formatting inconsistency in githooks.txt

[Andreas Heiduk]

Am 12.04.2018 um 21:36 schrieb Martin Ågren:
> On 11 April 2018 at 23:08, Andreas Heiduk  wrote:
>> - reflow some paragraphs
>> ---
>>  Documentation/githooks.txt | 14 +++---
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> I have reviewed the resulting githooks.txt. See the diff below for two
> more instances that I found. For the second hunk, I have difficulties
> parsing that paragraph, but I still claim those should be backticks and
> *git* read-tree...
>
> Martin

I've added the fixes for the next reroll and put you into a Reviewed-by
Trailer :-)

Andreas

Re: branch --contains / tag --merged inconsistency

[SZEDER Gábor]

Szia Feri,

> I'm moving the IRC discussion here, because this might be a bug report
> in the end.  So, kindly try these steps (103 MB free space required):
> 
> $ git clone https://github.com/ClusterLabs/pacemaker.git && cd pacemaker
> [...]
> $ git branch --contains Pacemaker-0.6.1
> * master
> $ git tag --merged master | fgrep Pacemaker-0.6
> Pacemaker-0.6.0
> Pacemaker-0.6.2
> Pacemaker-0.6.3
> Pacemaker-0.6.4
> Pacemaker-0.6.5
> Pacemaker-0.6.6
> 
> Notice that Pacemaker-0.6.1 is missing from the output.  Kind people on
> IRC didn't find a quick explanation, and we all had to go eventually.
> Is this expected behavior?  Reproduced with git 2.11.0 and 2.17.0.

The commit pointed to by the tag Pacemaker-0.6.1 and its parent have a
serious clock skew, i.e. they are a few months older than their parents:

$ git log --format='%h %ad %cd%d%n%s' --date=short 
Pacemaker-0.6.1^..47a8ef4c
47a8ef4ce 2008-02-15 2008-02-15
Low: TE: Logging - display the op's magic field for unexpected and foreign 
events
b9cfcd6b4 2007-12-10 2007-12-10 (tag: Pacemaker-0.6.2)
haresources2cib.py: set default-action-timeout to the default (20s)
52e7793e0 2007-12-10 2007-12-10
haresources2cib.py: update ra parameters lists
dea277271 2008-02-14 2008-02-14
Medium: Build: Turn on snmp support in rpm packages (patch from MATSUDA, 
Daiki)
f418742fe 2008-02-14 2008-02-14
Low: Build: Update the .spec file with the one used by build service
ccfa716a5 2008-02-14 2008-02-14
Medium: SNMP: Allow the snmp subagent to be built (patch from MATSUDA, 
Daiki)
50f0ade2d 2008-02-14 2008-02-14
Low: Build: Update last release number
90f11667f 2008-02-14 2008-02-14
Medium: Tools: Make sure the autoconf variables in haresources2cib are 
expanded
9d2383c46 2008-02-11 2008-02-11 (tag: Pacemaker-0.6.1)
High: cib: Ensure the archived file hits the disk before returning

(branch|tag|describe|...) (--contains|--merged) use the commit timestamp
information as a heuristic to avoid traversing parts of the history,
which makes these operations, especially on big histories, an order of
magnitude or two faster.  Yeah, commit timestamps can't always be
trusted, but skewed commits are rare, and skewed commits with this much
skew are even rarer.

I'm not sure how (or if it's at all possible) to turn off this
timestamp-based optimisation.

FWIW, much work is being done on a cached commit graph including commit
generation numbers, which will solve this issue both correctly and more
efficiently.  Perhaps it will already be included in the next release.

Hello My Dear Friend,

[Mrs.Zainab Ahmed]

I have a business proposal in the tune of $10.2 Million USD for you to
handle with me. I have opportunity to transfer this abandon fund to
your bank account in your country which belongs to our client.

I am inviting you in this transaction where this money can be shared
between us at ratio of 60/40% and help the needy around us don’t be
afraid of anything I am with you I will instruct you what you will do
to maintain this fund.

Please kindly contact me with your information's if you are interested
in this transaction for more details.

Your Name:..
Your Bank Name:.
Your Account Number:...
Your Telephone Number:
Your Country And Address:
Your Age And Sex:...

Thanks
Mrs.Zainab Ahmed,

branch --contains / tag --merged inconsistency

[Ferenc Wágner]

Hi,

I'm moving the IRC discussion here, because this might be a bug report
in the end.  So, kindly try these steps (103 MB free space required):

$ git clone https://github.com/ClusterLabs/pacemaker.git && cd pacemaker
[...]
$ git branch --contains Pacemaker-0.6.1
* master
$ git tag --merged master | fgrep Pacemaker-0.6
Pacemaker-0.6.0
Pacemaker-0.6.2
Pacemaker-0.6.3
Pacemaker-0.6.4
Pacemaker-0.6.5
Pacemaker-0.6.6

Notice that Pacemaker-0.6.1 is missing from the output.  Kind people on
IRC didn't find a quick explanation, and we all had to go eventually.
Is this expected behavior?  Reproduced with git 2.11.0 and 2.17.0.
-- 
Thanks,
Feri

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Elijah Newren]

Hi Dsco,

On Fri, Apr 27, 2018 at 12:23 AM, Johannes Schindelin
 wrote:
> Hi,

>
> Guys, you argued long and hard that one config setting (diff.renames)
> should magically imply another one (merge.renames), on the basis that they
> essentially do the same.

I apologize for any frustration; I've probably caused a good deal of
it by repeatedly catching or noticing things after one review and then
bringing them up later.  I just didn't catch it all at first.

Your restatement of the basis of the argument doesn't match my
rationale, though.  My reasoning was that (1) the configuration
options exist to allow the user to specify how much of a performance
cost they're willing to pay, (2) the two options are separate because
some users might want to configure one more loosely or tightly than
the other, but (3) many users will want to just specify once how much
they are willing to pay without being forced to repeat themselves for
different parts of git (diff, merge, possible future commands).

I'll add to my former basis by stating that I think the diffstat at
the end of the merge should be controlled by these variables, even if
that's not implemented as part of Ben's series.  And both of those
configuration options clearly feed into whether that diffstat should
be doing rename or copy or no detection.  While they could be kept
separate options, forcing us to somehow decide which one overrides
which, I think it's much more natural if we already have merge.renames
inheriting from and overriding diff.renames.

> And now you are suggesting that they *cannot* be essentially the same? Are
> you agreeing on such a violation of the Law of Least Surprise?
>
> Please, make up your mind. Inheriting merge.renames from diff.renames is
> "magic" enough to be puzzling. Introducing that auto-demoting is just
> simply creating a bad user experience. Please don't.

>From my view, resolve and octopus merges auto-demote diff.renames and
merge.renames to false.  In fact, they optimize the work involved in
that demotion by not even checking the value.  I think demoting "copy"
to "true" in the recursive merge machinery is no more surprising than
that is.  You can find more details to this argument in the portion of
my email that you responded to but snipped out.

But to add to that argument, if auto-demotion is a violation of the
Law of Least Surprise, as you claim, then naming this option
merge.renames is also a violation of the Law of Least Surprise.  You
would instead need to rename it to something like
merge.recursive.renames.  (And then when a new merge strategy comes
along that also does renames, we'll need to add a merge.ort.renames.)

> It is fine, of course, to admit at this stage that the whole magic idea of
> letting merge.renames inherit from diff.renames was only half thought
> through (we're in reviewing stage right now for the purpose of weighing
> alternative approaches and trying to come up with the best solution after
> all) and that we should go with Ben's original approach, which has the
> rather huge and dramatic advantage of being very, very clear and easy to
> understand to the user. We could even document that (and why) the 'copy'
> value in merge.renames is not allowed for the time being.

It was only half thought through, yes, at least by me.  But the more I
think through it, the more I like the inheritance personally.  I see
no problem with the demotion and think the inheritance has the
advantage of being easier to understand, because I see your proposal
as causing questions like:
  - "Why does merge.renameLimit inherit from diff.renameLimit but
merge.renames doesn't from diff.renames?"
  - "Why can't I just specify in one place that I {am, am not} willing
to pay for {full, both copy and} rename detection wherever it makes
sense?"
  - "How do I control the detection for the diffstat at the end of the merge?"


Hope that helps,
Elijah

[PATCH v1] perf/bisect_run_script: disable codespeed

[Christian Couder]

When bisecting a performance regression using a config file,
`./bisect_regression --config my_perf.conf` for example, the
config file can contain Codespeed configuration which would
instruct the 'aggregate.perl' script called by the 'run'
script to output results in the Codespeed format and maybe
to try to send this output to a Codespeed server.

This in unfortunate because the 'bisect_run_script' relies
on the regular output from 'aggregate.perl' to mesure
performance, so let's disable Codespeed output and sending
results to a Codespeed server.

Signed-off-by: Christian Couder 
---
 t/perf/bisect_run_script | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/t/perf/bisect_run_script b/t/perf/bisect_run_script
index 038255df4b..3ebaf15521 100755
--- a/t/perf/bisect_run_script
+++ b/t/perf/bisect_run_script
@@ -24,6 +24,12 @@ 
result_file="$info_dir/perf_${script_number}_${bisect_head}_results.txt"
 GIT_PERF_DIRS_OR_REVS="$bisect_head"
 export GIT_PERF_DIRS_OR_REVS
 
+# Don't use codespeed
+GIT_PERF_CODESPEED_OUTPUT=
+GIT_PERF_SEND_TO_CODESPEED=
+export GIT_PERF_CODESPEED_OUTPUT
+export GIT_PERF_SEND_TO_CODESPEED
+
 ./run "$script" >"$result_file" 2>&1 || die "Failed to run perf test '$script'"
 
 rtime=$(sed -n 
"s/^$script_number\.$test_number:.*\([0-9]\+\.[0-9]\+\)(.*).*\$/\1/p" 
"$result_file")
-- 
2.17.0.257.g28b659db43

[ANNOUNCE] Gitwin: Git Server for Windows with SSH/HTTP(S) transport and Gitweb

[tk]

Hi all,

This is a ONE-TIME announcement of Gitwin - a Git Server for Windows:

Gitwin is a packaging of Git, OpenSSH, Nginx and many other related tools to 
make it a ready-to-use
solution as a secure git repository on Windows. It supports SSH and HTTP(S) 
transports as well as
Gitweb with Highlighter and Gravatar support. A free edition is available.

Please visit https://itefix.net/gitwin for more information

Kind regards
Tevfik Karagulle
Itefix.net

Re: git merge banch w/ different submodule revision

[Middelschulte, Leif]

Hi,

firstofall: thank all of you for your feedback.

Am Donnerstag, den 26.04.2018, 17:19 -0700 schrieb Elijah Newren:
> On Thu, Apr 26, 2018 at 3:49 AM, Middelschulte, Leif
>  wrote:
> > Hi,
> > 
> > we're using git-flow as a basic development workflow. However, doing so 
> > revealed unexpected merge-behavior by git.
> > 
> > Assume the following setup:
> > 
> > - Repository `S` is sourced by repository `p` as submodule `s`
> > - Repository `p` has two branches: `feature_x` and `develop`
> > - The revisions sourced via the submodule have a linear history
> > 
> > 
> > * 1c1d38f (feature_x) update submodule revision to b17e9d9
> > > * 3290e69 (HEAD -> develop) update submodule revision to 0598394
> > > /
> > 
> > * cd5e1a5 initial submodule revision
> > 
> > 
> > Problem case: Merge either branch into the other
> > 
> > Expected behavior: Merge conflict.
> > 
> > Actual behavior: Auto merge without conflicts.
> > 
> > Note 1: A merge conflict does occur, if the sourced revisions do *not* have 
> > a linear history
> > 
> > Did I get something wrong about how git resolves merges? Shouldn't git be 
> > like: "hey, you're trying to merge two different contents for the same 
> > line" (the submodule's revision)
> 
> Hard to say without saying what commit was referenced for the
> submodule in the merge-bases for the two repositories you have.  In
> the basic case..
> 
> If branch A and branch B have different commits checked out in the
> submodule, say:
>A: deadbeef
>B: ba5eba11
> 
> then it's not clear whether there's a conflict or not.  The merge-base
> (the common point of history) matters.  So, for example if the
> original version (which I'll refer to as 'O") had:
>   O: deadbeef
> 
> then you would say, "Oh, branch A made no change to this submodule but
> B did.  So let's go with what B has."  Conversely, of O had ba5eba11,
> then you'd go the other way.
> 
> But, there is some further smarts in that if either A or B point at
> commits that contain the other in their history and both contain the
> commit that O points at, then you can just do a fast-forward update to
> the newest.
> 
> 
> You didn't tell us how the merge-base (cd5e1a5 from the diagram you
> gave) differed in your example here between the two repositories.  In
> fact, the non-linear case could have several merge-bases, in which
> case they all become potentially relevant (as does their merge-bases
> since at that point you'll trigger the recursive portion of
> merge-recursive).  Giving us that info might help us point out what
> happened, though if either the fast-forward logic comes into play or
> the recursive logic gets in the mix, then we may need you to provide a
> testcase (or access to the repo in question) in order to explain it
> and/or determine if you've found a bug.

I placed two reositories here: 
https://gitlab.com/foss-contributions/git-examples/network/develop
The access should be public w/o login.

If you prefer the examples to be placed somewhere else, let me know.

> 
> Does that help?

I guess it's somehow understandable that it tries to be more smart about things 
wrt submodules.

However, I believe that there should be some kind of choice here. Not giving 
*any* notice, makes testing feature-branches hell.

I hope the provided example exhibits the challenge.


BR,

Leif
> 
> Elijah
> 

Re: In some rebases, `exec git -C ...` has wrong working directory

[Johannes Schindelin]

Hi William,

On Thu, 26 Apr 2018, William Chargin wrote:

> Here is a repro script:
> 
> #!/bin/sh
> set -eux
> git --version
> tmpdir="$(mktemp -d)"
> cd "${tmpdir}"
> mkdir target repo
> cd repo
> git init
> touch file; git add file
> git commit -m "Initial commit"
> git rebase HEAD --exec "git -C ${tmpdir}/target init"

We do take pains to pass the GIT_DIR to the exec'ed command, and it is the
GIT_DIR of the worktree in which the rebase runs.

> The end of this script prints something like
> 
> Executing: git -C /tmp/tmp.gd2q51jO93/target init
> Reinitialized existing Git repository in /tmp/tmp.gd2q51jO93/repo/.git/
> Successfully rebased and updated refs/heads/master.

So that is actually what I expected.

I might even have one or two scripts relying on that feature, where
something like

exec git -C /somewhere/else show HEAD:some-file >some-other-file

is executed, and works.

> But this is wrong: the repository should be initialized in `target`, not
> reinitialized in `repo`.
> 
> Notes:
> 
>   - This propagates to subprocesses: if you run `exec make test` and
> your test suite ends up calling `git -C`, then the same problem
> occurs.
> 
>   - Substituting `rebase --root` for `rebase HEAD` causes the problem to
> go away.
> 
>   - The `rebase HEAD` exec context adds the `GIT_DIR` environment
> variable, and this is sufficient to reproduce the problem:
> running `GIT_DIR="$PWD" git -C /tmp/target init` puts the repo in
> the current working directory. The `rebase --root` context adds no
> such environment variable. (You can use `--exec 'env >tempfile'` to
> verify these.)
> 
> My `git --version` is 2.16.2.

Even `pu` has the same behavior.

I do not think that we can sensibly *remove* GIT_DIR from the environment
variables passed to the exec'ed command, as we have been doing that for
ages, and some scripts (as demonstrated above) started relying on that
behavior. So if we changed it, we would break backwards-compatibility,
which is something we try to avoid very much in Git.

Maybe you could a contribute a patch to the documentation? Something that
tells people who use the `--exec` option that they want to prefix their
command with `unset GIT_DIR; ` when their command wants to work with a
different Git repository than the current one?

Ciao,
Johannes

Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)

[Eric Sunshine]

On Wed, Apr 25, 2018 at 4:37 AM, Junio C Hamano  wrote:
> * tg/worktree-add-existing-branch (2018-04-25) 4 commits
>  - worktree: teach "add" to check out existing branches
>  - worktree: factor out dwim_branch function
>  - worktree: improve message when creating a new worktree
>  - worktree: remove extra members from struct add_opts
>
>  "git worktree add" learned to check out an existing branch.
>
>  Is this ready for 'next'?

I just gave v9 (which you have queued) a final full review and found
it satisfactory (and gave my Reviewed-by:), so yes, it seems ready for
'next'.

Thanks.

Re: [PATCH v9 0/4] worktree: teach "add" to check out existing branches

[Eric Sunshine]

On Tue, Apr 24, 2018 at 5:56 PM, Thomas Gummerer  wrote:
> Thanks Eric for the review and the suggestions on the previous round.
>
> Changes since the previous round:
>
> - UNLEAK new_branch after it was xstrndup'd
> - update the commit message of 2/4 according to Eric's suggestions
> - make force_new_branch a boolean flag in
>   print_preparing_worktree_line, instead of using it as the branch
>   name.  Instead use new_branch as the new branch name everywhere in
>   that function.
> - get rid of the ckeckout_existing_branch flag

Thanks. I did another full review of all the patches and played around
with the new functionality again for good measure. Everything looked
good, and I think the patches are now ready to graduate.

For what it's worth, the entire series is:

Reviewed-by: Eric Sunshine 

Re: [PATCH v3 2/3] merge: Add merge.renames config setting

[Johannes Schindelin]

Hi,

On Thu, 26 Apr 2018, Elijah Newren wrote:

> On Thu, Apr 26, 2018 at 7:23 PM, Junio C Hamano  wrote:
> > Ben Peart  writes:
> >
> >> Color me puzzled. :)  The consensus was that the default value for
> >> merge.renames come from diff.renames.  diff.renames supports copy
> >> detection which means that merge.renames will inherit that value.  My
> >> assumption was that is what was intended so when I reimplemented it, I
> >> fully implemented it that way.
> >>
> >> Are you now requesting to only use diff.renames as the default if the
> >> value is true or false but not if it is copy?  What should happen if
> >> diff.renames is actually set to copy?  Should merge silently change
> >> that to true, display a warning, error out, or something else?  Do you
> >> have some other behavior for how to handle copy being inherited from
> >> diff.renames you'd like to see?
> >>
> >> Can you write the documentation that clearly explains the exact
> >> behavior you want?  That would kill two birds with one stone... :)
> >
> > I think demoting from copy to rename-only is a good idea, at least
> > for now, because I do not believe we have figured out what we want
> > to happen when we detect copied files are involved in a merge.
> >
> > But I am not sure if we even want to fail merge.renames=copy as an
> > invalid configuration.  So my gut feeling of the best solution to
> > the above is to do something like:
> >
> >  - whether the configuration comes from diff.renames or
> >merge.renames, turn *.renames=copy to true inside the merge
> >recursive machinery.
> >
> >  - document the fact in "git merge-recursive" documentation (or "git
> >merge" documentation) to say "_currently_ asking for rename
> >detection to find copies and renames will do the same
> >thing---copies are ignored", impliying "this might change in the
> >future", in the BUGS section.
> 
> Yes, I agree.

Guys, you argued long and hard that one config setting (diff.renames)
should magically imply another one (merge.renames), on the basis that they
essentially do the same.

And now you are suggesting that they *cannot* be essentially the same? Are
you agreeing on such a violation of the Law of Least Surprise?

Please, make up your mind. Inheriting merge.renames from diff.renames is
"magic" enough to be puzzling. Introducing that auto-demoting is just
simply creating a bad user experience. Please don't.

It is fine, of course, to admit at this stage that the whole magic idea of
letting merge.renames inherit from diff.renames was only half thought
through (we're in reviewing stage right now for the purpose of weighing
alternative approaches and trying to come up with the best solution after
all) and that we should go with Ben's original approach, which has the
rather huge and dramatic advantage of being very, very clear and easy to
understand to the user. We could even document that (and why) the 'copy'
value in merge.renames is not allowed for the time being.

Ciao,
Dscho

Re: What's cooking in git.git (Apr 2018, #03; Wed, 25)

[Johannes Schindelin]

Hi,

On Thu, 26 Apr 2018, Derrick Stolee wrote:

> On 4/25/2018 1:43 PM, Brandon Williams wrote:
> > On 04/25, Ævar Arnfjörð Bjarmason wrote:
> > > > * bw/protocol-v2 (2018-03-15) 35 commits
> > > >   (merged to 'next' on 2018-04-11 at 23ee234a2c)

[... snip ...]

> > > >   (this branch is used by bw/server-options.)
> > > >
> > > >   The beginning of the next-gen transfer protocol.
> > > >
> > > >   Will cook in 'next'.
> > >
> > > With a month & 10 days of no updates & this looking stable it would
> > > be great to have it in master sooner than later to build on top of
> > > it in the 2.18 window.
> >
> > I personally think that this series is ready to graduate to master but
> > I mentioned to Junio off-list that it might be a good idea to wait
> > until it has undergone a little more stress testing.  We've been in
> > the process of trying to get this rolled out to our internal server
> > but due to a few roadblocks and people being out of the office its
> > taken a bit longer than I would have liked to get it up and running.
> > I'm hoping in another few days/a week I'll have some more data on live
> > traffic.  At that point I think I'll be more convinced that it will be
> > safe to merge it.
> >
> > I may be overly cautions but I'm hoping that we can get this right
> > without needing to do another protocol version bump for a very long
> > time.  Technically using v2 is hidden behind an "experimental" config
> > flag so we should still be able to tweak it after the fact if we
> > absolutely needed to, but I'd like to avoid that if necessary.
> 
> There's no testing better than production. I think if we have an
> opportunity to test with realistic traffic, we should take advantage.
> 
> But I also agree that this series looks stable.
> 
> I realize it's hard to communicate both "this series is ready to merge" and "I
> appreciate your caution."

To add to Stolee's comment: in our (day job) team, we are huge fans of
"Feature Flags". We blog plenty about it, GitHub uses it in their
Scientist approach, and even here in the Git project, we managed to use
the paradigm: remember the transition from the scripted version of
`difftool` to the builtin version? That was also done via a Feature Flag.

In the same vein, I would treat your config setting as a Feature Flag, and
I would be very much in favor of having the code in production, with that
feature flag defaulting to "off", and with a description that tells people
very clearly that not only the flag is experimental, but that we might
need to change the protocol, still, before v2 goes final.

I would then add an entry to Git for Windows' installer's "Experimental
Options" section, to give it a bit more exposure (mainly to verify that
the v2 client <-> v1 server connection works flawlessly), also as opt-in.

How does that sound?

Ciao,
Dscho

Re: [PATCH v2 5/5] builtin/config: introduce `color` type specifier

[Eric Sunshine]

On Thu, Apr 26, 2018 at 1:58 AM, Taylor Blau  wrote:
> [...]
> For consistency, let's introduce `--type=color` and encourage its use
> with `--default` together over `--get-color` alone.
>
> Signed-off-by: Taylor Blau 
> ---
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> @@ -228,6 +232,8 @@ Valid ``'s include:
> output it as the ANSI color escape sequence to the standard
> output.  The optional `default` parameter is used instead, if
> there is no color configured for `name`.
> ++
> +`--type=color [--default=]` is preferred over `--get-color`.

Rather than augmenting the documentation of --get-color like this, I
wonder if it would instead make more sense to replace it entirely,
like this:

--get-color name []
Deprecated alias for `--type=color [--default=]`.

Not necessarily worth a re-roll; could be done as a follow-up if
someone cares enough.

> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> @@ -931,6 +931,36 @@ test_expect_success 'get --expiry-date' '
> +test_expect_success 'get --type=color' '
> +   rm .git/config &&

I would feel more confident about the robustness of this test if it
instead used 'rm -f .git/config', as is already done elsewhere in this
test script.

> +   git config foo.color "red" &&
> +   git config --get --type=color foo.color >actual.raw &&
> +   test_decode_color actual &&
> +   echo "" >expect &&
> +   test_cmp expect actual
> +'
> +
> +test_expect_success 'set --type=color' '
> +   rm .git/config &&

Ditto.

> +   git config --type=color foo.color "red" &&
> +   test_cmp expect .git/config
> +'

Re: [PATCH 2/5] builtin/config.c: support `--type=` as preferred alias for `--type`

[Eric Sunshine]

On Thu, Apr 26, 2018 at 2:00 AM, Taylor Blau  wrote:
> On Thu, Apr 26, 2018 at 02:25:44PM +0900, Junio C Hamano wrote:
>> Taylor Blau  writes:
>>
>> > Subject: Re: [PATCH 2/5] builtin/config.c: support `--type=` as 
>> > preferred alias for `--type`
>>
>> I'd retitle while queuing, as the last 'type' is a placeholder for
>> concrete types like  above.
>
> Good idea. I amended v2 in this fashion.

Suggested previously[1], as well.

[1]: 
https://public-inbox.org/git/capig+ctw5zxkgocnombwim4p+9irpbyuvoagq0xujlb0ttx...@mail.gmail.com/