[Re-send PATCH v7 00/12] Deprecate .git/info/grafts

[Johannes Schindelin]

[Resent the cover letter, as I did not see it on public-inbox...]

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 v6:

- Made --convert-graft-file issue a mere warning (instead of an error) when
  a graft leaves the parents unchanged, and is hence unnecessary.

- Modified the regression test for --convert-graft-file to succeed even when
  the GPG prerequisite is unmet (thanks, Gábor!).

- Reassured by Stefan's feedback, I refrained from reinstating the contrib/
  script.


Johannes Schindelin (12):
  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: prepare create_graft() for converting graft files wholesale
  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 | 234 --
 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|  28 +++
 14 files changed, 274 insertions(+), 118 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-v7
Fetch-It-Via: git fetch https://github.com/dscho/git deprecate-grafts-v7

Interdiff vs v6:
 diff --git a/builtin/replace.c b/builtin/replace.c
 index 35394ec1874..a87fca045be 100644
 --- a/builtin/replace.c
 +++ b/builtin/replace.c
 @@ -429,7 +429,7 @@ static int check_mergetags(struct commit *commit, int 
argc, const char **argv)
return for_each_mergetag(check_one_mergetag, commit, _data);
  }
  
 -static int create_graft(int argc, const char **argv, int force)
 +static int create_graft(int argc, const char **argv, int force, int gentle)
  {
struct object_id old_oid, new_oid;
const char *old_ref = argv[0];
 @@ -471,8 +471,13 @@ static int create_graft(int argc, const char **argv, int 
force)
  
strbuf_release();
  
 -  if (!oidcmp(_oid, _oid))
 +  if (!oidcmp(_oid, _oid)) {
 +  if (gentle) {
 +  warning("graft for '%s' unnecessary", 
oid_to_hex(_oid));
 +  return 0;
 +  }
return error("new commit is the same as the old one: '%s'", 
oid_to_hex(_oid));
 +  }
  
return replace_object_oid(old_ref, _oid, "replacement", _oid, 
force);
  }
 @@ -492,7 +497,7 @@ static int convert_graft_file(int force)
continue;
  
argv_array_split(, buf.buf);
 -  if (args.argc && create_graft(args.argc, args.argv, force))
 +  if (args.argc && create_graft(args.argc, args.argv, force, 1))
strbuf_addf(, "\n\t%s", buf.buf);
argv_array_clear();
}
 @@ -583,7 +588,7 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
if (argc < 1)
usage_msg_opt("-g needs at least one argument",
  git_replace_usage, options);
 -  return create_graft(argc, argv, force);
 +  return create_graft(argc, argv, force, 0);
  
case MODE_CONVERT_GRAFT_FILE:
if (argc != 0)
 diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
 index bed86a0af3d..d174bfed309 100755
 --- a/t/t6050-replace.sh
 +++ b/t/t6050-replace.sh
 @@ -445,6 +445,14 @@ test_expect_success GPG '--graft on a commit with a 
mergetag' '
  '
  
  test_expect_success '--convert-graft-file' '
 +  git checkout -b with-graft-file &&
 +  test_commit root2 &&
 +  git reset --hard root2^ &&
 +  test_commit root1 &&
 +  test_commit after-root1 &&
 +  test_tick &&
 +  git merge -m merge-root2 root2 &&
 +
: add and convert graft file &&
   

[PATCH v7 12/12] 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.36.gdf4ca5fb72a

[PATCH v7 10/12] 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.36.gdf4ca5fb72a

[PATCH v7 09/12] 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.36.gdf4ca5fb72a

[PATCH v7 11/12] 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.36.gdf4ca5fb72a

[PATCH v7 02/12] 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 v7 01/12] 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.36.gdf4ca5fb72a

[PATCH v7 05/12] replace: prepare create_graft() for converting graft files wholesale

[Johannes Schindelin]

When converting all grafts in a graft file to replace refs, and one of
them happens to leave the original commit's parents unchanged, we do not
want to error out. Instead, we would like to issue a warning.

Prepare the create_graft() function for such a use case by adding a
`gentle` parameter. If set, we do not return an error when the replace ref
is unchanged, but a mere warning.

Signed-off-by: Johannes Schindelin 
---
 builtin/replace.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index e57d3d187ed..64f58112701 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -428,7 +428,7 @@ static int check_mergetags(struct commit *commit, int argc, 
const char **argv)
return for_each_mergetag(check_one_mergetag, commit, _data);
 }
 
-static int create_graft(int argc, const char **argv, int force)
+static int create_graft(int argc, const char **argv, int force, int gentle)
 {
struct object_id old_oid, new_oid;
const char *old_ref = argv[0];
@@ -470,8 +470,13 @@ static int create_graft(int argc, const char **argv, int 
force)
 
strbuf_release();
 
-   if (!oidcmp(_oid, _oid))
+   if (!oidcmp(_oid, _oid)) {
+   if (gentle) {
+   warning("graft for '%s' unnecessary", 
oid_to_hex(_oid));
+   return 0;
+   }
return error("new commit is the same as the old one: '%s'", 
oid_to_hex(_oid));
+   }
 
return replace_object_oid(old_ref, _oid, "replacement", _oid, 
force);
 }
@@ -547,7 +552,7 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
if (argc < 1)
usage_msg_opt("-g needs at least one argument",
  git_replace_usage, options);
-   return create_graft(argc, argv, force);
+   return create_graft(argc, argv, force, 0);
 
case MODE_LIST:
if (argc > 1)
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

[PATCH v7 08/12] 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.36.gdf4ca5fb72a

[PATCH v7 07/12] 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 | 28 
 1 file changed, 28 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index c630aba657e..d174bfed309 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -444,4 +444,32 @@ test_expect_success GPG '--graft on a commit with a 
mergetag' '
git replace -d $HASH10
 '
 
+test_expect_success '--convert-graft-file' '
+   git checkout -b with-graft-file &&
+   test_commit root2 &&
+   git reset --hard root2^ &&
+   test_commit root1 &&
+   test_commit after-root1 &&
+   test_tick &&
+   git merge -m merge-root2 root2 &&
+
+   : 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.36.gdf4ca5fb72a

[PATCH v7 06/12] 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 64f58112701..a87fca045be 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
@@ -481,6 +482,38 @@ static int create_graft(int argc, const char **argv, int 
force, int gentle)
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, 1))
+   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;
@@ -492,6 +525,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[] = {
@@ -499,6 +533,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")),
@@ -521,7 +556,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);
 
@@ -554,6 +590,12 @@ int cmd_replace(int argc, const char **argv, const char 
*prefix)
  git_replace_usage, options);
return create_graft(argc, argv, force, 0);
 
+   case MODE_CONVERT_GRAFT_FILE:
+   if (argc != 0)
+   usage_msg_opt("--convert-graft-file takes no argument",
+ git_replace_usage, options);
+ 

[PATCH v7 04/12] 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 v7 03/12] 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.36.gdf4ca5fb72a

Re: [PATCH v4 02/10] commit: add generation number to struct commmit

[Jakub Narebski]

Derrick Stolee  writes:

> The generation number of a commit is defined recursively as follows:
>
> * If a commit A has no parents, then the generation number of A is one.
> * If a commit A has parents, then the generation number of A is one
>   more than the maximum generation number among the parents of A.

Very minor nitpick: it would be more readable wrapped differently:

  * If a commit A has parents, then the generation number of A is
one more than the maximum generation number among parents of A.

Very minor nitpick: possibly "parents", not "the parents", but I am
not native English speaker.

>
> Add a uint32_t generation field to struct commit so we can pass this
> information to revision walks. We use three special values to signal
> the generation number is invalid:
>
> GENERATION_NUMBER_INFINITY 0x
> GENERATION_NUMBER_MAX 0x3FFF
> GENERATION_NUMBER_ZERO 0
>
> The first (_INFINITY) means the generation number has not been loaded or
> computed. The second (_MAX) means the generation number is too large to
> store in the commit-graph file. The third (_ZERO) means the generation
> number was loaded from a commit graph file that was written by a version
> of git that did not support generation numbers.

Good explanation; I wonder if we want to have it in some shortened form
also in comments, and not only in the commit message.

>
> Signed-off-by: Derrick Stolee 
> ---
>  alloc.c| 1 +
>  commit-graph.c | 2 ++
>  commit.h   | 4 
>  3 files changed, 7 insertions(+)

I have reordered patches to make it easier to review.

> diff --git a/commit.h b/commit.h
> index 23a3f364ed..aac3b8c56f 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -10,6 +10,9 @@
>  #include "pretty.h"
>  
>  #define COMMIT_NOT_FROM_GRAPH 0x
> +#define GENERATION_NUMBER_INFINITY 0x
> +#define GENERATION_NUMBER_MAX 0x3FFF
> +#define GENERATION_NUMBER_ZERO 0

I wonder if it wouldn't be good to have some short in-line comments
explaining those constants, or a block comment above them.

>  
>  struct commit_list {
>   struct commit *item;
> @@ -30,6 +33,7 @@ struct commit {
>*/
>   struct tree *maybe_tree;
>   uint32_t graph_pos;
> + uint32_t generation;
>  };
>  
>  extern int save_commit_buffer;

All right, simple addition of the new field.  Nothing to go wrong here.

Sidenote: With 0x7FFF being (if I am not wrong) maximum graph_pos
and maximum number of nodes in commit graph, we won't hit 0x3FFF
generation number limit for all except very, very linear histories.

>
> diff --git a/alloc.c b/alloc.c
> index cf4f8b61e1..e8ab14f4a1 100644
> --- a/alloc.c
> +++ b/alloc.c
> @@ -94,6 +94,7 @@ void *alloc_commit_node(void)
>   c->object.type = OBJ_COMMIT;
>   c->index = alloc_commit_index();
>   c->graph_pos = COMMIT_NOT_FROM_GRAPH;
> + c->generation = GENERATION_NUMBER_INFINITY;
>   return c;
>  }

All right, start with initializing it with "not from commit-graph" value
after allocation.

>  
> diff --git a/commit-graph.c b/commit-graph.c
> index 70fa1b25fd..9ad21c3ffb 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -262,6 +262,8 @@ static int fill_commit_in_graph(struct commit *item, 
> struct commit_graph *g, uin
>   date_low = get_be32(commit_data + g->hash_len + 12);
>   item->date = (timestamp_t)((date_high << 32) | date_low);
>  
> + item->generation = get_be32(commit_data + g->hash_len + 8) >> 2;
> +

I guess we should not worry about these "magical constants" sprinkled
here, like "+ 8" above.

Let's examine how it goes, taking a look at commit-graph-format.txt
in Documentation/technical/commit-graph-format.txt

 * The first H (g->hash_len) bytes are for the OID of the root tree.
 * The next 8 bytes are for the positions of the first two parents [...]

So 'commit_data + g->hash_len + 8' is our offset from the start of
commit data.  All right.

  * The next 8 bytes store the generation number of the commit and
the commit time in seconds since EPOCH.  The generation number
uses the higher 30 bits of the first 4 bytes. [...]

The higher 30 bits of the 4 bytes, which is 32 bits, means that we need
to shift 32-bit value 2 bits right, so that we get lower 30 bits of
32-bit value.  All right.

  All 4-byte numbers are in network order.

Shouldn't it be ntohl() to convert from network order to host order, and
not get_be32()?  I guess they are the same (network order is big-endian
order), and get_be32() is what rest of git uses...

Looks all right.

>   pptr = >parents;
>  
>   edge_value = get_be32(commit_data + g->hash_len);

[PATCH] tests: introduce test_unset_prereq, for debugging

[Johannes Schindelin]

While working on the --convert-graft-file test, I missed that I was
relying on the GPG prereq, by using output of test cases that were only
run under that prereq.

For debugging, it was really convenient to force that prereq to be
unmet, but there was no easy way to do that. So I came up with a way,
and this patch reflects the cleaned-up version of that way.

For convenience, the following two methods are now supported ways to
pretend that a prereq is not met:

test_set_prereq !GPG

and

test_unset_prereq GPG

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/test-unset-prereq-v1
Fetch-It-Via: git fetch https://github.com/dscho/git test-unset-prereq-v1
 t/test-lib-functions.sh | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 7d620bf2a9a..76cd6630f29 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -278,8 +278,20 @@ write_script () {
 # The single parameter is the prerequisite tag (a simple word, in all
 # capital letters by convention).
 
+test_unset_prereq () {
+   ! test_have_prereq "$1" ||
+   satisfied_prereq="${satisfied_prereq% $1 *} ${satisfied_prereq#* $1 }"
+}
+
 test_set_prereq () {
-   satisfied_prereq="$satisfied_prereq$1 "
+   case "$1" in
+   !*)
+   test_unset_prereq "${1#!}"
+   ;;
+   *)
+   satisfied_prereq="$satisfied_prereq$1 "
+   ;;
+   esac
 }
 satisfied_prereq=" "
 lazily_testable_prereq= lazily_tested_prereq=

base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d
-- 
2.17.0.windows.1.36.gdf4ca5fb72a

Re: [PATCH v5 0/5] Convert some stash functionality to a builtin

[Paul-Sebastian Ungureanu]

Hello Joel,


Since there seems to be interest from GSOC students who want to
work on converting builtins, I figured I should finish what I
have that works now so they could build on top of it.


First of all, I must thank you for submitting this series of patches. It 
is a great starting point to convert 'git stash'.


I would like to continue your work on "git stash" if that is fine with 
you. I will continue to build on top of it, starting with applying some 
patches in order to implement what was already suggested in this thread. 
 During the summer, I am planning to finish this process and, 
hopefully, have a 100% C, built-in 'git stash'.


Best regards,
Paul

Re: [RFC PATCH v3] Teach remote add the --remote-tags option

[Wink Saville]

I forgot to mention that I added the code to error out
if --remote-tags is used with "tracking" (-t | --track) or
mirror:
  +   if ((fetch_tags == TAGS_SET_REMOTE) && mirror)
  +   die(_("specifying --remote-tags makes no sense with
--mirror"));
  +   if ((fetch_tags == TAGS_SET_REMOTE) && track.nr)
  +   die(_("specifying --remote-tags makes no sense with
-t or --track"));

Re: [PATCH 2/2] unpack_trees_options: free messages when done

[Elijah Newren]

Hi Martin,

On Sat, Apr 28, 2018 at 4:32 AM, Martin Ågren  wrote:
> From: Elijah Newren 
>
> Hi Elijah,
>
> [Since this is leaving the topic of rename-detection in favour of
> leak-plugging, I'm shortening the cc-list a bit.]
>
>> So, instead, I'd like to see something like the below
>> (built on top of my series):
>
> Thanks a lot. I now have the below patch in my tree as a preparatory
> part of a three-patch series on top of your series. Since the gist of
> this patch is entirely your creation, is it ok if I place your Author:
> and Signed-off-by: on it? Credit where credit is due.

Sure, I'm fine with either that or an Original-patch-by attribution.

Anyway, thanks for fleshing it out with the commit message and
handling the early return cases.  And for tackling the
setup_unpack_trees_porcelain() memory leak.

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

[Michał Górny]

W dniu sob, 28.04.2018 o godzinie 16∶23 +0200, użytkownik Duy Nguyen
napisał:
> On Thu, Apr 26, 2018 at 4:46 PM, Michał Górny  wrote:
> > For the record, we're using this with ebuilds and respective cache files
> > (which are expensive to generate).  We are using separate repository
> > which combines sources and cache files to keep the development
> > repository clean.  I have researched different solutions for this but
> > git turned out the best option for incremental updates for us.
> > 
> > Tarballs are out of question, unless you expect users to fetch >100 MiB
> > every time, and they are also expensive to update.  Deltas of tarballs
> > are just slow and require storing a lot of extra data.  Rsync is not
> > very efficient at frequent updates, and has significant overhead
> > on every run.  With all its disadvantages, git is still something that
> > lets our users fetch updates frequently with minimal network overhead.
> 
> I assume you're talking about the metadata directory in gentoo-x86
> repo. This specific case could be solved by renaming metadata to
> _metadata or something to put it on the top. "git checkout" always
> updates files in strcmp(path) order. This guarantees time(_metadata)
> <= time(ebuild) for all ebuilds without any extra touching (either in
> git or in a post-checkout hook)

We can't really rename it without breaking compatibility with all
package managers out there.  Preparing to do such a major change for
the sake of abusing implementation detail of git doesn't look like
a worthwhile idea.

> 
> The behavior has been this way since forever and as far as I can tell
> very unlikely to change at least for branch switching (major changes
> involved around the index). It's a bit easier to accidentally change
> how "git checkout -- path" works though. I don't know if we could just
> make this checkout order a promise and guarantee not to break it
> though. For it it does not sound like it adds extra maintenance
> burden.

-- 
Best regards,
Michał Górny

Re: [RFC PATCH v3] Teach remote add the --remote-tags option

[Wink Saville]

In this version I changed "+refs/tags/*:refs/remotes/tags/%s/*"
back to "+refs/tags/*:refs/remote-tags/%s/*" as Junio originally
suggested. This way don't have remotename collisions as Bryan
pointed out.

Since I don't like "--prefix-tags" I changed the option to
"--remote-tags" but obviously open to anything else.

I think jacob.keller (+ demerphq) suggestion is probably good,
but for the moment just using remote-tags as seems simplest.

[RFC PATCH v3] Teach remote add the --remote-tags option

[Wink Saville]

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

For example:
  $ git remote add -f --remote-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  -> refs/remote-tags/gbenchmark/v0.0.9
   * [new tag] v0.1.0  -> refs/remote-tags/gbenchmark/v0.1.0
   * [new tag] v1.0.0  -> refs/remote-tags/gbenchmark/v1.0.0
   * [new tag] v1.1.0  -> refs/remote-tags/gbenchmark/v1.1.0
   * [new tag] v1.2.0  -> refs/remote-tags/gbenchmark/v1.2.0
   * [new tag] v1.3.0  -> refs/remote-tags/gbenchmark/v1.3.0
   * [new tag] v1.4.0  -> refs/remote-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/remote-tags/gbenchmark/*
tagopt = --remote-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 | 44 
 remote.c |  2 ++
 3 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-remote.txt b/Documentation/git-remote.txt
index 4feddc0293..fc983c2ff1 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] [--tags | --remote-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 --remote-tags to import them
+to refs/remote-tags//.
++
+With `--remote-tags` option, `git fetch ` imports every tag from the
+remote repository to refs/remote-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..07832113e9 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] [--tags | 
--remote-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_REMOTE = 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/remote-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 

Re: [PATCH v4 01/10] ref-filter: fix outdated comment on in_commit_list

[Jakub Narebski]

Derrick Stolee  writes:

> The in_commit_list() method does not check the parents of
> the candidate for containment in the list. Fix the comment
> that incorrectly states that it does.
>
> Reported-by: Jakub Narebski 
> Signed-off-by: Derrick Stolee 
> ---
>  ref-filter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index cffd8bf3ce..aff24d93be 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1582,7 +1582,7 @@ static int in_commit_list(const struct commit_list 
> *want, struct commit *c)
>  }
>  
>  /*
> - * Test whether the candidate or one of its parents is contained in the list.
> + * Test whether the candidate is contained in the list.
>   * Do not recurse to find out, though, but return -1 if inconclusive.
>   */
>  static enum contains_result contains_test(struct commit *candidate,

All right. Always good to have comment and code match.


FYI: the contains_test() function described in this comment only checks
the candidate, and never access candidate commit parents.  All
recursion, which naturally includes checking parents, is in the
contains_tag_algo().

I guess that the code was refactored, but comment had not been changed.

-- 
Jakub Narębski

Re: [PATCH v4 00/10] Compute and consume generation numbers

[Jakub Narebski]

Derrick Stolee  writes:

> As promised, here is the diff from v3.

What is this strange string "   " in place of tabs in the interdiff?
" " here is Unicode Character 'NO-BREAK SPACE' (U+00A0).

Though it doesn't matter for viewing, my newsreader (Gnus from GNU
Emacs) thinks that it is worth notifying about when replying.

Also, it looks like at least in one place the diff got line-wrapped.

> Thanks,
> -Stolee
>
> -- >8 --
>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 7e1da6c6ea..b819756946 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1148,6 +1148,7 @@ int cmd_merge(int argc, const char **argv, const
> char *prefix)
>     branch = branch_to_free = resolve_refdup("HEAD", 0, _oid,
> NULL);
>     if (branch)
>     skip_prefix(branch, "refs/heads/", );
> +
>     init_diff_ui_defaults();
>     git_config(git_merge_config, NULL);
>
> @@ -1156,7 +1157,6 @@ int cmd_merge(int argc, const char **argv, const
> char *prefix)
>     else
>     head_commit = lookup_commit_or_die(_oid, "HEAD");
>
> -
>     if (branch_mergeoptions)
>     parse_branch_merge_options(branch_mergeoptions);
>     argc = parse_options(argc, argv, prefix, builtin_merge_options,

Whitespace fixes, all right.

> diff --git a/commit-graph.c b/commit-graph.c
> index 21e853c21a..aebd242def 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -257,7 +257,7 @@ static int fill_commit_in_graph(struct commit
> *item, struct commit_graph *g, uin
>     uint32_t *parent_data_ptr;
>     uint64_t date_low, date_high;
>     struct commit_list **pptr;
> -   const unsigned char *commit_data = g->chunk_commit_data +
> GRAPH_DATA_WIDTH * pos;
> +   const unsigned char *commit_data = g->chunk_commit_data +
> (g->hash_len + 16) * pos;
>
>     item->object.parsed = 1;
>     item->graph_pos = pos;

This was accidental change in v3 (unrelated to the changes in commit it
were in).  Though I wonder if the symbolic constant route is not better
- though as separate standalone commit.

> @@ -304,7 +304,7 @@ static int find_commit_in_graph(struct commit
> *item, struct commit_graph *g, uin
>     *pos = item->graph_pos;
>     return 1;
>     } else {
> -   return bsearch_graph(commit_graph,
> &(item->object.oid), pos);
> +   return bsearch_graph(g, &(item->object.oid), pos);
>     }
>  }
>

Fixup for a commit, that was sent in separate fixup email in v3.  All
right.

Though I wonder if it wouldn't be better to call global variable
'the_commit_graph' to avoid such errors in the future...

> @@ -312,10 +312,10 @@ int parse_commit_in_graph(struct commit *item)
>  {
>     uint32_t pos;
>
> -   if (item->object.parsed)
> -   return 0;
>     if (!core_commit_graph)
>     return 0;
> +   if (item->object.parsed)
> +   return 1;
>     prepare_commit_graph();
>     if (commit_graph && find_commit_in_graph(item, commit_graph, ))
>     return fill_commit_in_graph(item, commit_graph, pos);

Fixed accidental flip-flopping about return value when
item->object.parsed.  I'd have to take a look at actual commits to say
whether I think it is all right or not.

> @@ -454,9 +454,8 @@ static void write_graph_chunk_data(struct hashfile
> *f, int hash_len,
>     else
>     packedDate[0] = 0;
>
> -   if ((*list)->generation != GENERATION_NUMBER_INFINITY) {
> +   if ((*list)->generation != GENERATION_NUMBER_INFINITY)
>     packedDate[0] |= htonl((*list)->generation << 2);
> -   }
>
>     packedDate[1] = htonl((*list)->date);
>     hashwrite(f, packedDate, 8);

Coding style change, to be more in line with CodingGuidelines, namely
that we usually do not use block for single-command in conditionals.

All right.

> diff --git a/commit.c b/commit.c
> index 9ef6f699bd..e2e16ea1a7 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -653,7 +653,7 @@ int compare_commits_by_gen_then_commit_date(const
> void *a_, const void *b_, void
>     else if (a->generation > b->generation)
>     return -1;
>
> -   /* use date as a heuristic when generataions are equal */
> +   /* use date as a heuristic when generations are equal */
>     if (a->date < b->date)
>     return 1;
>     else if (a->date > b->date)

Fixed typo in comment.  All right.

> @@ -1078,7 +1078,7 @@ int in_merge_bases_many(struct commit *commit,
> int nr_reference, struct commit *
>     }
>
>     if (commit->generation > min_generation)
> -   return 0;
> +   return ret;
>
>     bases = paint_down_to_common(commit, nr_reference, reference,
> commit->generation);
>     if (commit->object.flags & PARENT2)

Unifying way of returning result (to one that was used before this
commit in this fragment of 

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

[Stefan Beller]

On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin
 wrote:
> 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"

I have the impression this is the line that is really well
explained in the commit message ("migrate to rebase
helper even when there is $rebase_root set")

The rest of the patch is covered as "a couple of places
where we adjust stdout to stderr"?

Makes sense,
Stefan

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

[Stefan Beller]

On Fri, Apr 27, 2018 at 3:31 PM, Johannes Schindelin
 wrote:
> 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)

This seems to be the counter part of write_author_script(*msg),
would it make sense to either rename this to read_author_script
or rename the counter part to write_author_ident ?

> +{
> +   char *p, *p2;
> +
> +   if (strbuf_read_file(buf, rebase_path_author_script(), 256) <= 0)

The 256 is a hint for read_file how to size the buffer initially.
If not given it defaults to 8k, which presumably is too much for
an author identity.



> +   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);

This part could be prefixed with
/* un-escape text: turn \\ into ' and remove single quotes. */

> +   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;

So here we have read GIT_AUTHOR_NAME, _EMAIL
and _DATE in that order and converted it to its form
"name  date" in a single line.

It would be better to invert the conditions and keep
the indentation level lower by:

if (!skip_prefix(...))
goto warning_and_return;
strbuf_splice(...);
...

I wondered if we want to factor out the conversion of
"author string in commit form" to "author information
in script form" into their own functions, and keep the reading
writing out of them. But then again we only need them in
these use cases for now, and such a 

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

[Thomas Gummerer]

On 04/27, Eric Sunshine wrote:
> 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.

Thanks for all your review work on this series!

> For what it's worth, the entire series is:
> 
> Reviewed-by: Eric Sunshine 

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

[Stefan Beller]

Hi Johannes,

On Fri, Apr 27, 2018 at 3:30 PM, Johannes Schindelin
 wrote:
> 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;
> +   }

This move is a verbatim move of the code below, except that
we need to add braces. For some reason I fantasize of using
the comma operator in C eventually (which we do not use in
our code base AFAICT), then we could leave out the braces
and have a

return error(...), NULL;

Anyway, this patch is
Reviewed-by: Stefan Beller 

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

[Stefan Beller]

Hi Philip,

On Sat, Apr 28, 2018 at 2:04 AM, Philip Oakley  wrote:
> From: "Johannes Schindelin" 
>>
>> The functionality is now implemented as `git replace
>> --convert-graft-file`.
>
>
> A rather late in the day thought: Should this go through the same
> deprecation dance?

I think contrib/ has other rules than the core
(c.f. `git log --oneline --no-merges --grep "contrib: remove"`)
which is why I'd consider the deprecation dance unneeded.

> At least it will catch those who arrive via random web advice!

Good point, unlike the other removals, this is not a dormant
project.

I would still think it is ok to remove it. Those who are looking
for this script, surely will look at the man page of git-replace
or are looking for the grafts. `man gitglossary` is already
sufficient:

grafts
   Grafts enables [...]

   Note that the grafts mechanism is outdated and can
   lead to problems transferring objects between
   repositories; see git-replace(1) for a more flexible
   and robust system to do the same thing.

Re: [PATCH v4 04/10] commit: use generations in paint_down_to_common()

[Jakub Narebski]

Jakub Narebski  writes:
> Junio C Hamano  writes:
>> Derrick Stolee  writes:
[...]
>>> +int compare_commits_by_gen_then_commit_date(const void *a_, const void 
>>> *b_, void *unused)
>>> +{
>>> +   const struct commit *a = a_, *b = b_;
>>> +
>>> +   /* newer commits first */
>>> +   if (a->generation < b->generation)
>>> +   return 1;
>>> +   else if (a->generation > b->generation)
>>> +   return -1;
>>
>> ... this does not check if a->generation is _ZERO or _INF.  
>>
>> Both being _MAX is OK (the control will fall through and use the
>> dates below).  One being _MAX and the other being a normal value is
>> also OK (the above comparisons will declare the commit with _MAX is
>> farther than less-than-max one from a root).
>>
>> Or is the assumption that if one has _ZERO, that must have come from
>> an ancient commit-graph file and none of the commits have anything
>> but _ZERO?
>
> There is stronger and weaker version of the negative-cut criteria based
> on generation numbers.
>
> The strong criteria:
>
>   if A != B and gen(A) <= gen(B), then A cannot reach B
>
> The weaker criteria:
>
>   if gen(A) < gen(B), then A cannot reach B
>
>
> Because commit-graph is closed under reachability, this means that
>
>   if A is in commit graph, and B is outside of it, then A cannot reach B
>
> If A is in commit graph, then either _MAX >= gen(A) >= 1,
> or gen(A) == _ZERO.  Because _INFINITY > _MAX > _ZERO, then we have
>
>   if _MAX >= gen(A) >= 1 || gen(A) == 0, and gen(B) == _INFINITY
>   then A cannot reach B
>
> which also fullfils the weaker criteria
>
>   if gen(A) < gen(B), then A cannot reach B
>
>
> If both A and B are outside commit-graph, i.e. gen(A) = gen(B) = _INFINITY,
> or if both A and B have gen(A) = gen(B) = _MAX,
> or if both A and B come from old commit graph with gen(A) = gen(B) =_ZERO,
> then we cannot say anything about reachability... and weak criteria
> also does not say anything about reachability.
>
>
> Maybe the following ASCII table would make it clear.
>
>  |  gen(B)
>  | :::
> gen(A)   | _INFINITY | _MAX | larger   | smaller  | _ZERO
> -+---+--+--+--+
> _INFINITY| = | >| >| >| >
> _MAX | < Nn  | =| >| >| >
> larger   | < Nn  | < Nn | = n  | >| >
> smaller  | < Nn  | < Nn | < Nn | = n  | >
> _ZERO| < Nn  | < Nn | < Nn | < Nn | =
>
> Here "n" denotes stronger condition, and "N" denotes weaker condition.
> We have _INFINITY > _MAX > larger > smaller > _ZERO.
>
>
> NOTE however that it is a *tradeoff*.  Using weaker criteria, with
> strict inequality, means that we don't need to handle _INFINITY, _MAX
> and _ZERO corner-cases in a special way; but it also means that we would
> walk slightly more commits than if we used stronger criteria, with less
> or equals.

Actually, if we look at the table above, it turns out that we can use
the stronger version of negative-cut criteria without special-casing all
the possible combinations.  Just use stronger criteria on normal range,
weaker criteria if any of generation numbers is special generation
number.

  if _MAX > gen(A) > _ZERO and
 _MAX > gen(B) > _ZERO then

if A != B and gen(A) <= gen(B) then
  A cannot reach B
else
  A can reach B

  else /* at least one special case */

if gen(A) < gen(B) then
  A cannot reach B
else
  A can reach B


NOTE that it specifically does not matter for created here
compare_commits_by_gen_then_commit_date(), as it requires strict
inequality for sorting - which using weak criteria explains why we don't
need any special cases in the code here.

Best,
-- 
Jakub Narębski

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

[Duy Nguyen]

On Thu, Apr 26, 2018 at 4:46 PM, Michał Górny  wrote:
> For the record, we're using this with ebuilds and respective cache files
> (which are expensive to generate).  We are using separate repository
> which combines sources and cache files to keep the development
> repository clean.  I have researched different solutions for this but
> git turned out the best option for incremental updates for us.
>
> Tarballs are out of question, unless you expect users to fetch >100 MiB
> every time, and they are also expensive to update.  Deltas of tarballs
> are just slow and require storing a lot of extra data.  Rsync is not
> very efficient at frequent updates, and has significant overhead
> on every run.  With all its disadvantages, git is still something that
> lets our users fetch updates frequently with minimal network overhead.

I assume you're talking about the metadata directory in gentoo-x86
repo. This specific case could be solved by renaming metadata to
_metadata or something to put it on the top. "git checkout" always
updates files in strcmp(path) order. This guarantees time(_metadata)
<= time(ebuild) for all ebuilds without any extra touching (either in
git or in a post-checkout hook)

The behavior has been this way since forever and as far as I can tell
very unlikely to change at least for branch switching (major changes
involved around the index). It's a bit easier to accidentally change
how "git checkout -- path" works though. I don't know if we could just
make this checkout order a promise and guarantee not to break it
though. For it it does not sound like it adds extra maintenance
burden.
-- 
Duy

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

[Johannes Schindelin]

Hi Philip,

On Sat, 28 Apr 2018, Philip Oakley wrote:

> From: "Johannes Schindelin" 
> > The functionality is now implemented as `git replace
> > --convert-graft-file`.
> 
> A rather late in the day thought: Should this go through the same
> deprecation dance?
> 
> I.e. replace the body of the script with the new `git
> replace --convert-graft-file` and echo (or die!) a warning message that this
> script is now deprecated and will be removed?
> 
> At least it will catch those who arrive via random web advice!

Originally, I did not even want to remove it... I just did as I was told
by Junio...

Ciao,
Dscho

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

[Johannes Schindelin]

Hi Gábor,

On Sat, 28 Apr 2018, SZEDER Gábor wrote:

> > 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.
>
> [... demonstrates how the new test fails without the GPG prereq ...]

Ouch. You're right. Will fix,
Dscho

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

[Johannes Schindelin]

Hi Stefan,

On Fri, 27 Apr 2018, Stefan Beller wrote:

> 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.

And that is indeed the case. The commit message would look something like
this:

# This is a combination of 3 commits.
# This is commit message #1:

Hello Stefan

This is A.

# This is commit message #2:

squash! A

Me again, Stefan. I am here to be squashed.

# The commit message #3 will be skipped:
#
# fixup! A

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

Nope.

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

No worries, you learned something today.

> > The diff is best viewed with --color-moved.
> 
> ... and web pages are "best viewed with IE 6.0" ;-)

That is what I had in mind writing that.

> 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.

I agree that the current iteration is no longer such obvious a move. I had
to add tons of stuff to fix the extra issues I found while working on v4.

But still, I found it super-helpful to see that the code was actually
moved, and where, because I essentially had to break up the nice sequence
of "is it clean? Yes? Then nothing to be done! No? Is HEAD to be amended?
Yes? No?" and basically build a matrix what to do in all combinations of
"clean? Amend HEAD?"

Ciao,
Dscho

Re: Branch deletion question / possible bug?

[Johannes Schindelin]

Hi,

On Sat, 28 Apr 2018, Philip Oakley wrote:

> From: "Jacob Keller" 
> > On Fri, Apr 27, 2018 at 5:29 PM, Tang (US), Pik S 
> > wrote:
> > > 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
> > >
> >
> > Are you running on a case-insensitive file system? What version of
> > git? I thought I recalled seeing commits to help avoid creating
> > branches of the same name with separate case when we know we're on a
> > file system which is case-insensitive..
> >
> > > 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.
> >
> > If we have not yet done this, I think we should. Long term this would
> > be fixed by using a separate format to store refs than the filesystem,
> > which has a few projects being worked on but none have been put into a
> > release.
> 
> Yes, this is an on-going problem on Windows and other case insentive
> systems. At the moment the branch name becomes embedded as a file name, so
> when Git requests details of a branch from the filesystem, it can get a case
> insensitive equivalent. Meanwhile, internally Git is checking for equality
> in a case sensitive [Linux] way with obvious consequences such as this - The
> most obvious being when there is no "*" current branch marker in the branch
> status list.
> 
> It's a bit tricky to fix (internally the name and the path are passed down
> different call chains), and depends on how one expects the case
> insensitivity to work - the kicker is when someone does an edit of the name
> via the file system and expects Git to cope (i.e. devs knowing, or think
> they know, too much detail ;-).
> 
> The refs can also get packed, so the "bad spelling" gets baked in.
> Ultimately it probably means that GfW and other systems will need  a case
> sensitivity check when opening paths...

FWIW I outlined what I think is the best route to fix this for good:

https://github.com/git-for-windows/git/issues/1623#issuecomment-380085257

Essentially, I think we should teach Git the trick to check the spelling
before calling lstat() in refs/files-backend.c.

To check the spelling, we would need an API to get the on-disk
representation of a given path. On Windows, I know this call. On Linux,
apparently canonicalize_file_name() might do the job, but that is a GNU
libc extension, and won't help us on macOS.

Any ideas?

Ciao,
Dscho

Re: [PATCH 2/2] unpack_trees_options: free messages when done

[Johannes Schindelin]

Hi Martin,

On Sat, 28 Apr 2018, Martin Ågren wrote:

> -->8--
> Subject: merge-recursive: provide pair of `unpack_trees_{start,finish}()`
> 
> Rename `git_merge_trees()` to `unpack_trees_start()` and extract the
> call to `discard_index()` into a new function `unpack_trees_finish()`.
> As a result, these are called early resp. late in `merge_trees()`,
> making the resource handling clearer. The next commit will expand on
> that, teaching `..._finish()` to free more memory. (So rather than
> moving the TODO-comment, just drop it, since it will be addressed soon
> enough.)
> 
> Also call `..._finish()` when `merge_trees()` returns early.

Looks good! It is missing a Signed-off-by: line, and you probably want to
start a new thread that also includes the "next commit", but other than
that it is pretty nice and ready for contributing, methinks.

Ciao,
Dscho

Re: [PATCH] http.c: shell command evaluation for extraheader

[Johannes Schindelin]

Hi Colin,

On Tue, 6 Mar 2018, Colin Arnott wrote:

> On March 5, 2018 1:47 PM, Johannes Schindelin  
> wrote:
> 
> > As the credential-helper is already intended for sensitive data, and
> > as it already allows to interact with a helper, I would strongly
> > assume that it would make more sense to try to extend that feature
> > (instead of the simple extraHeader one).
> 
> To confirm you are suggesting that the credential struct, defined in 
> credential.h, be extended to include a headers array, like so:
> --- a/credential.h
> +++ b/credential.h
> @@ -15,6 +15,7 @@ struct credential {
> char *protocol;
> char *host;
> char *path;
> +   char **headers
>  };
>  
>  #define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }

I think that was what I had in mind, yes.

> > This would also help alleviate all the quoting/dequoting issues involved
> > with shell scripting.
> > 
> > Besides, the http.extraHeader feature was designed to accommodate all
> > kinds of extra headers, not only authentication ones (and indeed, the
> > authentication was only intended for use in build agents, where both
> > environment and logging can be controlled rather tightly).
> 
> I realise that my examples are scoped for auth, but I can conceive of
> other mutating headers that are not explicitly authentication related,
> and could benefit from shell execution before fetch, pull, push actions.

I can conceive of yet another use case that would benefit from shell
execution in these scenarios: attacks. That is why I am extremely hesitant
to go that route.

But then, I am not the maintainer of Git. If you can convince him, you're
good to go.

> > I also see that in your implementation, only the extraHeader value is
> > evaluated, without any access to the rest of the metadata (such as URL,
> > and optionally specified user).
> >
> > It would probably get a little more complicated than a shell script to
> > write a credential-helper that will always be asked to generate an
> > authentication, but I think even a moderate-level Perl script could be
> > used for that, and it would know the URL and user for which the
> > credentials are intended...
> 
> You are correct; the scope provided by http..* is enough to meet my
> use cases, however I agree the lack of access to metadata limits what
> can be done within in the context of the shell, and makes the case for a
> credential-helper implementation stronger. I think there is something to
> be said about the simplicity and user-friendliness of allowing shell
> scripts for semi-complex config options, but authentication is a task
> that should be handled well and centrally, thus extending the
> credential-api makes sense.

Yes, I agree.

Ciao,
Johannes

Re: [Bug] git log --show-signature print extra carriage return ^M

[Johannes Schindelin]

Hi Larry,


On Tue, 6 Mar 2018, Larry Hunter wrote:

> The same ^M is shown in the output of tutorial
> 
> 
> https://www.geekality.net/2017/08/23/setting-up-gpg-signing-for-gitgithub-on-windows/
> 
> at item "4. Verify commit was signed"


Please understand that it would be helpful if you could take the lead on
resolving this issue.

If you need pointers how to get started with fixing this behavior, please
just tell us where you got stuck, so we can help you get un-stuck.

Ciao,
Johannes

> I confirm the output is right (no ^M characters) with commands
> 
> git verify-commit HEAD
> git -p verify-commit HEAD
> git verify-commit --v HEAD
> git verify-commit --raw HEAD
> 
> and wrong (ending with ^M characters) with
> 
> git  log --show-signature -1 HEAD
> git  -p log --show-signature -1 HEAD
> 
> I need gpg version 2.1 or greater to generate a gpg key for my windows
> system, as stated by the github documentation:
> 
> https://help.github.com/articles/generating-a-new-gpg-key/
> 
> that saves my keys in ~/AppData/Roaming/GnuPG.
> 
> 2018-03-05 15:29 GMT+01:00 Johannes Schindelin :
> > Hi Larry,
> >
> > On Sun, 4 Mar 2018, Larry Hunter wrote:
> >
> >> There is bug using "git log --show-signature" in my installation
> >>
> >> git 2.16.2.windows.1
> >> gpg (GnuPG) 2.2.4
> >> libgcrypt 1.8.2
> >
> > The gpg.exe shipped in Git for Windows should say something like this:
> >
> > $ gpg --version
> > gpg (GnuPG) 1.4.22
> > Copyright (C) 2015 Free Software Foundation, Inc.
> > License GPLv3+: GNU GPL version 3 or later
> > 
> > This is free software: you are free to change and redistribute it.
> > There is NO WARRANTY, to the extent permitted by law.
> >
> > Home: ~/.gnupg
> > Supported algorithms:
> > Pubkey: RSA, RSA-E, RSA-S, ELG-E, DSA
> > Cipher: IDEA, 3DES, CAST5, BLOWFISH, AES, AES192, AES256, TWOFISH,
> > CAMELLIA128, CAMELLIA192, CAMELLIA256
> > Hash: MD5, SHA1, RIPEMD160, SHA256, SHA384, SHA512, SHA224
> > Compression: Uncompressed, ZIP, ZLIB, BZIP2
> >
> > Therefore, the GNU Privacy Guard version you use is not the one shipped
> > and supported by the Git for Windows project.
> >
> >> that prints (with colors) an extra ^M (carriage return?) at the end of
> >> the gpg lines. As an example, the output of "git log --show-signature
> >> HEAD" looks like:
> >>
> >> $ git log --show-signature HEAD
> >> commit 46c490188ebd216f20c454ee61108e51b481844e (HEAD -> master)
> >> gpg: Signature made 03/04/18 16:53:06 ora solare Europa occidentale^M
> >> gpg:using RSA key ...^M
> >> gpg: Good signature from "..." [ultimate]^M
> >> Author: ... <...>
> >> Date:   Sun Mar 4 16:53:06 2018 +0100
> >> ...
> >>
> >> To help find a fix, I tested the command "git verify-commit HEAD" that
> >> prints (without colors) the same lines without extra ^M characters.
> >>
> >> $ git verify-commit HEAD
> >> gpg: Signature made 03/04/18 16:53:06 ora solare Europa occidentale
> >> gpg:using RSA key ...
> >> gpg: Good signature from "..." [ultimate]
> >
> > My guess is that the latter command simply does not go through the pager
> > while the former does.
> >
> > Do you see the ^M in the output of `git -p verify-commit HEAD`?
> >
> > Ciao,
> > Johannes
> 

Re: Git Stash Creates sh.exe.stackdump (STATUS_STACK_OVERFLOW)

[Johannes Schindelin]

Hi Derek,

On Fri, 16 Feb 2018, Derek Foulk wrote:

> An issue has arisen sometime between 2.16.1.windows.1 and 2.16.1.windows.4 in 
> Git.
> 
> When you execute `git stash` commands (stash/pop/apply?), a sh.exe.stackdump 
> file is generated that contains the following:
> 
> Exception: STATUS_STACK_OVERFLOW at rip=7FFB2C324B97
> rax=0010 rbx= rcx=0010
> rdx=000C rsi=AAE0 rdi=7FFB23B8D4F0
> r8 =AA18 r9 =AA50 r10=A000
> r11=FFE03E70 r12=B500 r13=0015C000
> r14=014C r15=
> rbp=AA50 rsp=AA08
> program=C:\Program Files\Git\usr\bin\sh.exe, pid 41552, thread unknown 
> (0x949C)
> [...]

I saw similar reports on the Git for Windows bug tracker. Probably the
most relevant one is this here:
https://github.com/git-for-windows/git/issues/1562

In essence, the culprit there seems to be an overzealous anti-malware
called Comodo Internet Security. Maybe your setup has the same problem?

Ciao,
Johannes

Re: [PATCH 2/2] unpack_trees_options: free messages when done

[Martin Ågren]

From: Elijah Newren 

Hi Elijah,

[Since this is leaving the topic of rename-detection in favour of 
leak-plugging, I'm shortening the cc-list a bit.]

> So, instead, I'd like to see something like the below
> (built on top of my series):

Thanks a lot. I now have the below patch in my tree as a preparatory
part of a three-patch series on top of your series. Since the gist of
this patch is entirely your creation, is it ok if I place your Author:
and Signed-off-by: on it? Credit where credit is due.

As you noted elsewhere [1], Ben is also working in this area. I'd be
perfectly happy to sit on these patches until both of your contributions
come through to master.

[1] 
https://public-inbox.org/git/CABPp-BFh=gl6rnbst2bgtynkij1z5tmgar1via5_vytef5e...@mail.gmail.com/

Martin

-->8--
Subject: merge-recursive: provide pair of `unpack_trees_{start,finish}()`

Rename `git_merge_trees()` to `unpack_trees_start()` and extract the
call to `discard_index()` into a new function `unpack_trees_finish()`.
As a result, these are called early resp. late in `merge_trees()`,
making the resource handling clearer. The next commit will expand on
that, teaching `..._finish()` to free more memory. (So rather than
moving the TODO-comment, just drop it, since it will be addressed soon
enough.)

Also call `..._finish()` when `merge_trees()` returns early.
---
 merge-recursive.c | 29 +++--
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 1de8dc1c53..e64102004a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -337,10 +337,10 @@ static void init_tree_desc_from_tree(struct tree_desc 
*desc, struct tree *tree)
init_tree_desc(desc, tree->buffer, tree->size);
 }
 
-static int git_merge_trees(struct merge_options *o,
-  struct tree *common,
-  struct tree *head,
-  struct tree *merge)
+static int unpack_trees_start(struct merge_options *o,
+ struct tree *common,
+ struct tree *head,
+ struct tree *merge)
 {
int rc;
struct tree_desc t[3];
@@ -378,6 +378,11 @@ static int git_merge_trees(struct merge_options *o,
return rc;
 }
 
+static void unpack_trees_finish(struct merge_options *o)
+{
+   discard_index(>orig_index);
+}
+
 struct tree *write_tree_from_memory(struct merge_options *o)
 {
struct tree *result = NULL;
@@ -3079,13 +3084,14 @@ int merge_trees(struct merge_options *o,
return 1;
}
 
-   code = git_merge_trees(o, common, head, merge);
+   code = unpack_trees_start(o, common, head, merge);
 
if (code != 0) {
if (show(o, 4) || o->call_depth)
err(o, _("merging of trees %s and %s failed"),
oid_to_hex(>object.oid),
oid_to_hex(>object.oid));
+   unpack_trees_finish(o);
return -1;
}
 
@@ -3138,20 +3144,15 @@ int merge_trees(struct merge_options *o,
 
hashmap_free(>current_file_dir_set, 1);
 
-   if (clean < 0)
+   if (clean < 0) {
+   unpack_trees_finish(o);
return clean;
+   }
}
else
clean = 1;
 
-   /* Free the extra index left from git_merge_trees() */
-   /*
-* FIXME: Need to also data allocated by setup_unpack_trees_porcelain()
-* tucked away in o->unpack_opts.msgs, but the problem is that only
-* half of it refers to dynamically allocated data, while the other
-* half points at static strings.
-*/
-   discard_index(>orig_index);
+   unpack_trees_finish(o);
 
if (o->call_depth && !(*result = write_tree_from_memory(o)))
return -1;
-- 
2.17.0

Re: Branch deletion question / possible bug?

[Philip Oakley]

From: "Jacob Keller" 

On Fri, Apr 27, 2018 at 5:29 PM, Tang (US), Pik S 
wrote:

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



Are you running on a case-insensitive file system? What version of
git? I thought I recalled seeing commits to help avoid creating
branches of the same name with separate case when we know we're on a
file system which is case-insensitive..


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.


If we have not yet done this, I think we should. Long term this would
be fixed by using a separate format to store refs than the filesystem,
which has a few projects being worked on but none have been put into a
release.


Yes, this is an on-going problem on Windows and other case insentive
systems. At the moment the branch name becomes embedded as a file name, so
when Git requests details of a branch from the filesystem, it can get a case
insensitive equivalent. Meanwhile, internally Git is checking for equality
in a case sensitive [Linux] way with obvious consequences such as this - The
most obvious being when there is no "*" current branch marker in the branch
status list.

It's a bit tricky to fix (internally the name and the path are passed down
different call chains), and depends on how one expects the case
insensitivity to work - the kicker is when someone does an edit of the name
via the file system and expects Git to cope (i.e. devs knowing, or think
they know, too much detail ;-).

The refs can also get packed, so the "bad spelling" gets baked in.
Ultimately it probably means that GfW and other systems will need  a case
sensitivity check when opening paths...

Philip


Thanks,
Jake




Thanks,

Pik Tang

تمكين سوق ابل

[ابو سيف Alrashdi UAE]

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

[Philip Oakley]

From: "Johannes Schindelin" 

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


A rather late in the day thought: Should this go through the same
deprecation dance?

I.e. replace the body of the script with the new `git
replace --convert-graft-file` and echo (or die!) a warning message that this
script is now deprecated and will be removed?

At least it will catch those who arrive via random web advice!

--
Philip


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

Re: Branch deletion question / possible bug?

[Jacob Keller]

On Fri, Apr 27, 2018 at 5:29 PM, Tang (US), Pik S  wrote:
> 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
>

Are you running on a case-insensitive file system? What version of
git? I thought I recalled seeing commits to help avoid creating
branches of the same name with separate case when we know we're on a
file system which is case-insensitive..

> 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.

If we have not yet done this, I think we should. Long term this would
be fixed by using a separate format to store refs than the filesystem,
which has a few projects being worked on but none have been put into a
release.

Thanks,
Jake

>
>
> Thanks,
>
> Pik Tang
>

Re: Fetching tags overwrites existing tags

[Jacob Keller]

On Fri, Apr 27, 2018 at 12:08 PM, Wink Saville  wrote:
> 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.

The only main concern I have with "remote" is that it is very similar
but not exactly the same as "remotes". Unfortunately, it is not
possible in *every* circumstance to use remotes.

Personally, I'd prefer we used "refs/remote//tags" rather
than "refs/remote/tags/", and possibly plan to migrate
from refs/remotes// to
refs/remote//heads/

This is mostly so that future additions of things like notes,
replaces, or really *any* refs would automatically drop into
"refs/remotes//",
which is a longer term goal I've had for a while (though i haven't
been able to put much time to it at present). Historically, I proposed
using "tracking" instead of "remote", but I am ok with any name we
choose as long as it doesn't create conflicts.

Thanks,
Jake

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

Re: git merge banch w/ different submodule revision

[Jacob Keller]

On Fri, Apr 27, 2018 at 5:24 PM, Elijah Newren  wrote:
> 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.

My interpretation of the counter example was that the two submodule
updates were not linear (i.e. one did not contain the other after
updating).

I could be wrong, so more clarification from Lief would be helpful in
illuminating the problem.

Thanks,
Jake

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

[Ferenc Wágner]

SZEDER Gábor  writes:

> 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:
> [...]
> (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.

Szia Gábor,

Thank you very much for the explanation!  Good to know there's no
serious problem or misunderstanding here, but a conscious algorithmic
choice being fooled by clock skew.  I wonder if there's a way such clock
skew can appear without actual committer clocks being off by months...

> 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.

Wonderful news, thanks for sharing it!
-- 
Regards,
Feri

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

[Duy Nguyen]

On Fri, Apr 27, 2018 at 11:08 PM, Marc Branchaud  wrote:
>> On Wed, Apr 25, 2018 at 5:18 PM, Marc Branchaud 
>> 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.

I send pathspecs, not paths. If you type "git checkout -- foo/" then I
send exactly "foo/" not every paths in it. You can always figure that
out with git-ls-files.

Sure this can still hit command length limit when you do "git checkout
-- foo/*" and have lots of files in foo just one more param from
hitting the limit, then the hook may hit the limit because we need
more command line arguments. But this is the corner case I don't think
we should really need to care about.

> 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.

Flexibility vs convenience I guess. A sample hook as template should
help the reinvention.

> 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.

This goes back to my post-checkout-modified proposal. If you're
writing to file, might as well reuse the index format. Then you can
read it with ls-files (which lets you decide path separator or even
quoting, I'm not sure) and it also provides some more info like file
hashes, access time...
-- 
Duy

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

[Duy Nguyen]

On Fri, Apr 27, 2018 at 11:08 PM, Elijah Newren  wrote:
> 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?

For this use case? Probably (assuming that timestamp precision does
not cause problems).

I'm more concerned about what info post-checkout hook should provide
but does not. Giving hook writer a way to get the list of updated
files lets them do more fancy stuff while providing checkout start
time probably only helps just this one case.

Providing start time in general for all hooks sounds like a good thing
though (and simple enough to implement).
-- 
Duy