Re: [PATCH] real_path: make real_path thread-safe

[Torsten Bögershausen]

On Wed, Dec 07, 2016 at 02:13:35PM -0800, Brandon Williams wrote:
> On 12/07, Junio C Hamano wrote:
> > Torsten Bögershausen  writes:
> > 
> > > But in any case it seems that e.g.
> > > //SEFVER/SHARE/DIR1/DIR2/..
> > > must be converted into
> > > //SEFVER/SHARE/DIR1
> > >
> > > and 
> > > \\SEFVER\SHARE\DIR1\DIR2\..
> > > must be converted into
> > > \\SEFVER\SHARE\DIR1
> > 
> > Additional questions that may be interesting are:
> > 
> > //A/B/../C  is it //A/C?  is it an error?
Yes, at least under Windows.
If I have e.g. a Raspi with SAMBA, I can put a git Repository here: 

//raspi/torsten/projects/git
If I use
git push //raspi/torsten/../junio/projects/git
that should be an error.

> > //A/B/../../C/D is it //C/D?  is it an error?
> > 

Same for
git push /raspi/../raspi2/torsten//projects/git


> 
> 
> Also is //.. the same as //?  I would assume so since /.. is /
> 
Under Windows //.. is simply illegal, I would say.
The documentation here
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx

Mentions these 2 examples, what to feed into the WIN32 file API:

a)
\\?\D:\very-long-path

b)
\\server\share\path\file"

c)
"\\?\UNC\server\share\path" 

So whatever we do, the ".." resoltion is only allowed to look at 
"very-long-path" or "path".

Some conversion may be done in mingw.c:
https://github.com/github/git-msysgit/blob/master/compat/mingw.c
So what I understand, '/' in Git are already converted into '\' if needed ?

It seams that we may wnat a function get_start_of_path(uncpath),
which returns:

get_start_of_path_win("//?/D:/very-long-path") "/very-long-path" 
get_start_of_path_win("//server/share/path/file")  "/path/file"
get_start_of_path_win("//?/UNC/server/share/path") "/path"
(I don't know if we need the variant with '\', but is would'n hurt):
get_start_of_path_win("?\\D:\\very-long-path") "\\very-long-path" 
get_start_of_path_win("server\\share\\path\\file")  "\\path\\file"
get_start_of_path_win("?\\UNC\\server\\share\\path") "\\path"

Then the non-windows version could simply return
get_start_of_path_non_win(something) something

Does this make sense ?


 

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Christian Couder]

Hi,

On Wed, Dec 7, 2016 at 7:36 PM, Stephan Beyer  wrote:
> Hi,
>
> On 12/06/2016 07:58 PM, Junio C Hamano wrote:
>
>>  (1) The third invocation of "cherry-pick" with "--abort" to get rid
>>  of the state from the unfinished cherry-pick we did previously
>>  is necessary, but the command does not notice that we resetted
>>  to a new branch AND we even did some other work there.  This
>>  loses end-user's work.
>>
>>  "git cherry-pick --abort" should learn from "git am --abort"
>>  that has an extra safety to deal with the above workflow.  The
>>  state from the unfinished "am" is removed, but the head is not
>>  rewound to avoid losing end-user's work.
>>
>>  You can try by replacing two instances of
>>
>>   $ git cherry-pick 0582a34f52..a94bb68397
>>
>>  with
>>
>>   $ git format-patch --stdout 0582a34f52..a94bb68397 | git am
>>
>>  in the above sequence, and conclude with "git am--abort" to see
>>  how much more pleasant and safe "git am --abort" is.
> Definitely. I'd volunteer to add that safety guard. (But (2) remains.)

That would be great if you could take care of that!

Thanks,
Christian.

Re: [PATCH v15 19/27] bisect--helper: `bisect_state` & `bisect_head` shell function in C

[Pranit Bauva]

Hey Stephan,

On Wed, Dec 7, 2016 at 5:24 AM, Stephan Beyer  wrote:
> Hi Pranit,
>
> On 12/06/2016 11:40 PM, Pranit Bauva wrote:
>> On Tue, Nov 22, 2016 at 5:42 AM, Stephan Beyer  wrote:
>>> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
 +static int bisect_state(struct bisect_terms *terms, const char **argv,
 + int argc)
 +{
 + const char *state = argv[0];
 +
 + get_terms(terms);
 + if (check_and_set_terms(terms, state))
 + return -1;
 +
 + if (!argc)
 + die(_("Please call `--bisect-state` with at least one 
 argument"));
>>>
>>> I think this check should move to cmd_bisect__helper. There are also the
>>> other argument number checks.
>>
>> Not really. After the whole conversion, the cmdmode will cease to
>> exists while bisect_state will be called directly, thus it is
>> important to check it here.
>
> Okay, that's a point.
> In that case, you should probably use "return error()" again and the
> message (mentioning "--bisect-state") does not make sense when
> --bisect-state ceases to exist.

True. Will change accordingly.

 +
 + if (argc == 1 && one_of(state, terms->term_good,
 + terms->term_bad, "skip", NULL)) {
 + const char *bisected_head = xstrdup(bisect_head());
 + const char *hex[1];
>>>
>>> Maybe:
>>> const char *hex;
>>>
 + unsigned char sha1[20];
 +
 + if (get_sha1(bisected_head, sha1))
 + die(_("Bad rev input: %s"), bisected_head);
>>>
>>> And instead of...
>>>
 + if (bisect_write(state, sha1_to_hex(sha1), terms, 0))
 + return -1;
 +
 + *hex = xstrdup(sha1_to_hex(sha1));
 + if (check_expected_revs(hex, 1))
 + return -1;
>>>
>>> ... simply:
>>>
>>> hex = xstrdup(sha1_to_hex(sha1));
>>> if (set_state(terms, state, hex)) {
>>> free(hex);
>>> return -1;
>>> }
>>> free(hex);
>>>
>>> where:
>>
>> Yes I am planning to convert all places with hex rather than the sha1
>> but not yet, maybe in an another patch series because currently a lot
>> of things revolve around sha1 and changing its behaviour wouldn't
>> really be a part of a porting patch series.
>
> I was not suggesting a change of behavior, I was suggesting a simple
> helper function set_state() to get rid of code duplication above and
> some lines below:
>
>>> ... And replace this:
>>>
 + const char **hex_string = (const char **) 
 [i].string;
 + if(bisect_write(state, *hex_string, terms, 0)) {
 + string_list_clear(, 0);
 + return -1;
 + }
 + if (check_expected_revs(hex_string, 1)) {
 + string_list_clear(, 0);
 + return -1;
 + }
>>>
>>> by:
>>>
>>> const char *hex_str = hex.items[i].string;
>>> if (set_state(terms, state, hex_string)) {
>>> string_list_clear(, 0);
>>> return -1;
>>> }
>
> See?

I can do this change of using set_state() keeping aside the sha1 to
hex and such change.

 @@ -184,8 +137,8 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
   state="$TERM_GOOD"
   fi

 - # We have to use a subshell because "bisect_state" can exit.
 - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
 + # We have to use a subshell because "--bisect-state" can 
 exit.
 + ( git bisect--helper --bisect-state $state 
 >"$GIT_DIR/BISECT_RUN" )
>>>
>>> The new comment is funny, but you don't need a subshell here any
>>> longer.
>>
>> True, but right now I didn't want to modify that part of the source
>> code to remove the comment. I will remove the comment all together
>> when I port bisect_run()
> For most of the patches, I was commenting on the current state, not on
> the big picture.
>
> Still I think that it is better to remove the comment and the subshell
> instead of making the comment weird ("yes the builtin can exit, but why
> do we need a subshell?" vs "yes the shell function calls exit, so we
> need a subshell because we do not want to exit this shell script")

Sure I will remove the comment.

Regards,
Pranit Bauva

[PATCHv6 6/7] move connect_work_tree_and_git_dir to dir.h

[Stefan Beller]

That function was primarily used by submodule code, but the function
itself is not inherently about submodules. In the next patch we'll
introduce relocate_git_dir, which can be used by worktrees as well,
so find a neutral middle ground in dir.h.

Signed-off-by: Stefan Beller 
---
 dir.c   | 26 ++
 dir.h   |  1 +
 submodule.c | 26 --
 submodule.h |  1 -
 4 files changed, 27 insertions(+), 27 deletions(-)

diff --git a/dir.c b/dir.c
index bfa8c8a9a5..8b74997c66 100644
--- a/dir.c
+++ b/dir.c
@@ -2748,3 +2748,29 @@ void untracked_cache_add_to_index(struct index_state 
*istate,
 {
untracked_cache_invalidate_path(istate, path);
 }
+
+/* Update gitfile and core.worktree setting to connect work tree and git dir */
+void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
+{
+   struct strbuf file_name = STRBUF_INIT;
+   struct strbuf rel_path = STRBUF_INIT;
+   char *real_git_dir = xstrdup(real_path(git_dir));
+   char *real_work_tree = xstrdup(real_path(work_tree));
+
+   /* Update gitfile */
+   strbuf_addf(_name, "%s/.git", work_tree);
+   write_file(file_name.buf, "gitdir: %s",
+  relative_path(real_git_dir, real_work_tree, _path));
+
+   /* Update core.worktree setting */
+   strbuf_reset(_name);
+   strbuf_addf(_name, "%s/config", real_git_dir);
+   git_config_set_in_file(file_name.buf, "core.worktree",
+  relative_path(real_work_tree, real_git_dir,
+_path));
+
+   strbuf_release(_name);
+   strbuf_release(_path);
+   free(real_work_tree);
+   free(real_git_dir);
+}
diff --git a/dir.h b/dir.h
index 97c83bb383..051674a431 100644
--- a/dir.h
+++ b/dir.h
@@ -335,4 +335,5 @@ struct untracked_cache *read_untracked_extension(const void 
*data, unsigned long
 void write_untracked_extension(struct strbuf *out, struct untracked_cache 
*untracked);
 void add_untracked_cache(struct index_state *istate);
 void remove_untracked_cache(struct index_state *istate);
+extern void connect_work_tree_and_git_dir(const char *work_tree, const char 
*git_dir);
 #endif
diff --git a/submodule.c b/submodule.c
index 66c5ce5a24..0bb50b4b62 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1222,32 +1222,6 @@ int merge_submodule(unsigned char result[20], const char 
*path,
return 0;
 }
 
-/* Update gitfile and core.worktree setting to connect work tree and git dir */
-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)
-{
-   struct strbuf file_name = STRBUF_INIT;
-   struct strbuf rel_path = STRBUF_INIT;
-   char *real_git_dir = xstrdup(real_path(git_dir));
-   char *real_work_tree = xstrdup(real_path(work_tree));
-
-   /* Update gitfile */
-   strbuf_addf(_name, "%s/.git", work_tree);
-   write_file(file_name.buf, "gitdir: %s",
-  relative_path(real_git_dir, real_work_tree, _path));
-
-   /* Update core.worktree setting */
-   strbuf_reset(_name);
-   strbuf_addf(_name, "%s/config", real_git_dir);
-   git_config_set_in_file(file_name.buf, "core.worktree",
-  relative_path(real_work_tree, real_git_dir,
-_path));
-
-   strbuf_release(_name);
-   strbuf_release(_path);
-   free(real_work_tree);
-   free(real_git_dir);
-}
-
 int parallel_submodules(void)
 {
return parallel_jobs;
diff --git a/submodule.h b/submodule.h
index d9e197a948..4e3bf469b4 100644
--- a/submodule.h
+++ b/submodule.h
@@ -65,7 +65,6 @@ int merge_submodule(unsigned char result[20], const char 
*path, const unsigned c
 int find_unpushed_submodules(unsigned char new_sha1[20], const char 
*remotes_name,
struct string_list *needs_pushing);
 int push_unpushed_submodules(unsigned char new_sha1[20], const char 
*remotes_name);
-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);
 int parallel_submodules(void);
 
 /*
-- 
2.11.0.rc2.30.gc512cbd.dirty

[PATCHv6 1/7] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

The current caller of connect_work_tree_and_git_dir passes
an absolute path for the `git_dir` parameter. In the future patch
we will also pass in relative path for `git_dir`. Extend the functionality
of connect_work_tree_and_git_dir to take relative paths for parameters.

We could work around this in the future patch by computing the absolute
path for the git_dir in the calling site, however accepting relative
paths for either parameter makes the API for this function much harder
to misuse.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 submodule.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/submodule.c b/submodule.c
index 6f7d883de9..66c5ce5a24 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char 
*work_tree, const char *git_dir)
 {
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
-   const char *real_work_tree = xstrdup(real_path(work_tree));
+   char *real_git_dir = xstrdup(real_path(git_dir));
+   char *real_work_tree = xstrdup(real_path(work_tree));
 
/* Update gitfile */
strbuf_addf(_name, "%s/.git", work_tree);
write_file(file_name.buf, "gitdir: %s",
-  relative_path(git_dir, real_work_tree, _path));
+  relative_path(real_git_dir, real_work_tree, _path));
 
/* Update core.worktree setting */
strbuf_reset(_name);
-   strbuf_addf(_name, "%s/config", git_dir);
+   strbuf_addf(_name, "%s/config", real_git_dir);
git_config_set_in_file(file_name.buf, "core.worktree",
-  relative_path(real_work_tree, git_dir,
+  relative_path(real_work_tree, real_git_dir,
 _path));
 
strbuf_release(_name);
strbuf_release(_path);
-   free((void *)real_work_tree);
+   free(real_work_tree);
+   free(real_git_dir);
 }
 
 int parallel_submodules(void)
-- 
2.11.0.rc2.30.gc512cbd.dirty

[PATCHv6 3/7] test-lib-functions.sh: teach test_commit -C

[Stefan Beller]

Specifically when setting up submodule tests, it comes in handy if
we can create commits in repositories that are not at the root of
the tested trash dir. Add "-C " similar to gits -C parameter
that will perform the operation in the given directory.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 t/test-lib-functions.sh | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..579e812506 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -157,16 +157,21 @@ debug () {
 GIT_TEST_GDB=1 "$@"
 }
 
-# Call test_commit with the arguments " [ [ []]]"
+# Call test_commit with the arguments
+# [-C ]  [ [ []]]"
 #
 # This will commit a file with the given contents and the given commit
 # message, and tag the resulting commit with the given tag name.
 #
 # , , and  all default to .
+#
+# If the first argument is "-C", the second argument is used as a path for
+# the git invocations.
 
 test_commit () {
notick= &&
signoff= &&
+   indir= &&
while test $# != 0
do
case "$1" in
@@ -176,21 +181,26 @@ test_commit () {
--signoff)
signoff="$1"
;;
+   -C)
+   indir="$2"
+   shift
+   ;;
*)
break
;;
esac
shift
done &&
+   indir=${indir:+"$indir"/} &&
file=${2:-"$1.t"} &&
-   echo "${3-$1}" > "$file" &&
-   git add "$file" &&
+   echo "${3-$1}" > "$indir$file" &&
+   git ${indir:+ -C "$indir"} add "$file" &&
if test -z "$notick"
then
test_tick
fi &&
-   git commit $signoff -m "$1" &&
-   git tag "${4:-$1}"
+   git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
+   git ${indir:+ -C "$indir"} tag "${4:-$1}"
 }
 
 # Call test_merge with the arguments " ", where 
-- 
2.11.0.rc2.30.gc512cbd.dirty

[PATCHv6 7/7] submodule: add absorb-git-dir function

[Stefan Beller]

When a submodule has its git dir inside the working dir, the submodule
support for checkout that we plan to add in a later patch will fail.

Add functionality to migrate the git directory to be absorbed
into the superprojects git directory.

The newly added code in this patch is structured such that other areas of
Git can also make use of it. The code in the submodule--helper is a mere
wrapper and option parser for the function
`absorb_git_dir_into_superproject`, that takes care of embedding the
submodules git directory into the superprojects git dir. That function
makes use of the more abstract function for this use case
`relocate_gitdir`, which can be used by e.g. the worktree code eventually
to move around a git directory.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt|  15 ++
 builtin/submodule--helper.c|  38 ++
 dir.c  |  12 +
 dir.h  |   3 ++
 git-submodule.sh   |   7 ++-
 submodule.c| 103 +
 submodule.h|   4 ++
 t/t7412-submodule-absorbgitdirs.sh | 101 
 8 files changed, 282 insertions(+), 1 deletion(-)
 create mode 100755 t/t7412-submodule-absorbgitdirs.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..918bd1d1bd 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,6 +22,7 @@ SYNOPSIS
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
 'git submodule' [--quiet] sync [--recursive] [--] [...]
+'git submodule' [--quiet] absorbgitdirs [--] [...]
 
 
 DESCRIPTION
@@ -245,6 +246,20 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
+absorbgitdirs::
+   If a git directory of a submodule is inside the submodule,
+   move the git directory of the submodule into its superprojects
+   `$GIT_DIR/modules` path and then connect the git directory and
+   its working directory by setting the `core.worktree` and adding
+   a .git file pointing to the git directory embedded in the
+   superprojects git directory.
++
+A repository that was cloned independently and later added as a submodule or
+old setups have the submodules git directory inside the submodule instead of
+embedded into the superprojects git directory.
++
+This command is recursive by default.
+
 OPTIONS
 ---
 -q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 33676a57cf..0108afac93 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,43 @@ static int resolve_remote_submodule_branch(int argc, 
const char **argv,
return 0;
 }
 
+static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
+{
+   int i;
+   struct pathspec pathspec;
+   struct module_list list = MODULE_LIST_INIT;
+   unsigned flags = ABSORB_GITDIR_RECURSE_SUBMODULES;
+
+   struct option embed_gitdir_options[] = {
+   OPT_STRING(0, "prefix", ,
+  N_("path"),
+  N_("path into the working tree")),
+   OPT_BIT(0, "--recursive", , N_("recurse into submodules"),
+   ABSORB_GITDIR_RECURSE_SUBMODULES),
+   OPT_END()
+   };
+
+   const char *const git_submodule_helper_usage[] = {
+   N_("git submodule--helper embed-git-dir [...]"),
+   NULL
+   };
+
+   argc = parse_options(argc, argv, prefix, embed_gitdir_options,
+git_submodule_helper_usage, 0);
+
+   gitmodules_config();
+   git_config(submodule_config, NULL);
+
+   if (module_list_compute(argc, argv, prefix, , ) < 0)
+   return 1;
+
+   for (i = 0; i < list.nr; i++)
+   absorb_git_dir_into_superproject(prefix,
+   list.entries[i]->name, flags);
+
+   return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -1094,6 +1131,7 @@ static struct cmd_struct commands[] = {
{"resolve-relative-url-test", resolve_relative_url_test, 0},
{"init", module_init, 0},
{"remote-branch", resolve_remote_submodule_branch, 0},
+   {"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/dir.c b/dir.c
index 8b74997c66..cc5729f733 100644
--- a/dir.c
+++ b/dir.c
@@ -2774,3 +2774,15 @@ void connect_work_tree_and_git_dir(const char 
*work_tree, const char *git_dir)
free(real_work_tree);
free(real_git_dir);
 }
+
+/*
+ * Migrate the git directory of the given path from old_git_dir to new_git_dir.
+ */
+void relocate_gitdir(const char *path, const char *old_git_dir, 

[PATCHv6 5/7] worktree: add function to check if worktrees are in use

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 worktree.c | 24 
 worktree.h |  7 +++
 2 files changed, 31 insertions(+)

diff --git a/worktree.c b/worktree.c
index 75db689672..2559f33846 100644
--- a/worktree.c
+++ b/worktree.c
@@ -406,3 +406,27 @@ const struct worktree *find_shared_symref(const char 
*symref,
 
return existing;
 }
+
+static int uses_worktree_internal(struct worktree **worktrees)
+{
+   int i;
+   for (i = 0; worktrees[i]; i++)
+   ; /* nothing */
+
+   free_worktrees(worktrees);
+   return i > 1;
+}
+
+int uses_worktrees(void)
+{
+   return uses_worktree_internal(get_worktrees(0));
+}
+
+int submodule_uses_worktrees(const char *path)
+{
+   struct worktree **worktrees = get_submodule_worktrees(path, 0);
+   if (!worktrees)
+   return 0;
+
+   return uses_worktree_internal(worktrees);
+}
diff --git a/worktree.h b/worktree.h
index 157fbc4a66..76027b1fd2 100644
--- a/worktree.h
+++ b/worktree.h
@@ -33,6 +33,13 @@ extern struct worktree **get_worktrees(unsigned flags);
 extern struct worktree **get_submodule_worktrees(const char *path,
 unsigned flags);
 
+/*
+ * Returns 1 if more than one worktree exists.
+ * Returns 0 if only the main worktree exists.
+ */
+extern int uses_worktrees(void);
+extern int submodule_uses_worktrees(const char *path);
+
 /*
  * Return git dir of the worktree. Note that the path may be relative.
  * If wt is NULL, git dir of current worktree is returned.
-- 
2.11.0.rc2.30.gc512cbd.dirty

[PATCHv6 2/7] submodule helper: support super prefix

[Stefan Beller]

Just like main commands in Git, the submodule helper needs
access to the superproject prefix. Enable this in the git.c
but have its own fuse in the helper code by having a flag to
turn on the super prefix.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/submodule--helper.c | 31 ---
 git.c   |  2 +-
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..33676a57cf 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,21 +1076,24 @@ static int resolve_remote_submodule_branch(int argc, 
const char **argv,
return 0;
 }
 
+#define SUPPORT_SUPER_PREFIX (1<<0)
+
 struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
+   int option;
 };
 
 static struct cmd_struct commands[] = {
-   {"list", module_list},
-   {"name", module_name},
-   {"clone", module_clone},
-   {"update-clone", update_clone},
-   {"relative-path", resolve_relative_path},
-   {"resolve-relative-url", resolve_relative_url},
-   {"resolve-relative-url-test", resolve_relative_url_test},
-   {"init", module_init},
-   {"remote-branch", resolve_remote_submodule_branch}
+   {"list", module_list, 0},
+   {"name", module_name, 0},
+   {"clone", module_clone, 0},
+   {"update-clone", update_clone, 0},
+   {"relative-path", resolve_relative_path, 0},
+   {"resolve-relative-url", resolve_relative_url, 0},
+   {"resolve-relative-url-test", resolve_relative_url_test, 0},
+   {"init", module_init, 0},
+   {"remote-branch", resolve_remote_submodule_branch, 0},
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
@@ -1100,9 +1103,15 @@ int cmd_submodule__helper(int argc, const char **argv, 
const char *prefix)
die(_("submodule--helper subcommand must be "
  "called with a subcommand"));
 
-   for (i = 0; i < ARRAY_SIZE(commands); i++)
-   if (!strcmp(argv[1], commands[i].cmd))
+   for (i = 0; i < ARRAY_SIZE(commands); i++) {
+   if (!strcmp(argv[1], commands[i].cmd)) {
+   if (get_super_prefix() &&
+   !(commands[i].option & SUPPORT_SUPER_PREFIX))
+   die("%s doesn't support --super-prefix",
+   commands[i].cmd);
return commands[i].fn(argc - 1, argv + 1, prefix);
+   }
+   }
 
die(_("'%s' is not a valid submodule--helper "
  "subcommand"), argv[1]);
diff --git a/git.c b/git.c
index efa1059fe0..98dcf6c518 100644
--- a/git.c
+++ b/git.c
@@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {
{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
{ "stripspace", cmd_stripspace },
-   { "submodule--helper", cmd_submodule__helper, RUN_SETUP },
+   { "submodule--helper", cmd_submodule__helper, RUN_SETUP | 
SUPPORT_SUPER_PREFIX},
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
{ "tag", cmd_tag, RUN_SETUP },
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
-- 
2.11.0.rc2.30.gc512cbd.dirty

[PATCHv6 4/7] worktree: get worktrees from submodules

[Stefan Beller]

In a later patch we want to move around the the git directory of
a submodule. Both submodules as well as worktrees are involved in
placing git directories at unusual places, so their functionality
may collide. To react appropriately to situations where worktrees
in submodules are in use, offer a new function to query the
worktrees for submodules.

Signed-off-by: Stefan Beller 
---
 worktree.c | 46 --
 worktree.h |  6 ++
 2 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/worktree.c b/worktree.c
index eb6121263b..75db689672 100644
--- a/worktree.c
+++ b/worktree.c
@@ -72,7 +72,7 @@ static void add_head_info(struct strbuf *head_ref, struct 
worktree *worktree)
 /**
  * get the main worktree
  */
-static struct worktree *get_main_worktree(void)
+static struct worktree *get_main_worktree(const char *git_common_dir)
 {
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -81,12 +81,12 @@ static struct worktree *get_main_worktree(void)
int is_bare = 0;
int is_detached = 0;
 
-   strbuf_add_absolute_path(_path, get_git_common_dir());
+   strbuf_add_absolute_path(_path, git_common_dir);
is_bare = !strbuf_strip_suffix(_path, "/.git");
if (is_bare)
strbuf_strip_suffix(_path, "/.");
 
-   strbuf_addf(, "%s/HEAD", get_git_common_dir());
+   strbuf_addf(, "%s/HEAD", git_common_dir);
 
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(_path, NULL);
@@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
return worktree;
 }
 
-static struct worktree *get_linked_worktree(const char *id)
+static struct worktree *get_linked_worktree(const char *git_common_dir,
+   const char *id)
 {
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -112,7 +113,7 @@ static struct worktree *get_linked_worktree(const char *id)
if (!id)
die("Missing linked worktree name");
 
-   strbuf_git_common_path(, "worktrees/%s/gitdir", id);
+   strbuf_addf(, "%s/worktrees/%s/gitdir", git_common_dir, id);
if (strbuf_read_file(_path, path.buf, 0) <= 0)
/* invalid gitdir file */
goto done;
@@ -125,7 +126,7 @@ static struct worktree *get_linked_worktree(const char *id)
}
 
strbuf_reset();
-   strbuf_addf(, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+   strbuf_addf(, "%s/worktrees/%s/HEAD", git_common_dir, id);
 
if (parse_ref(path.buf, _ref, _detached) < 0)
goto done;
@@ -167,7 +168,8 @@ static int compare_worktree(const void *a_, const void *b_)
return fspathcmp((*a)->path, (*b)->path);
 }
 
-struct worktree **get_worktrees(unsigned flags)
+static struct worktree **get_worktrees_internal(const char *git_common_dir,
+   unsigned flags)
 {
struct worktree **list = NULL;
struct strbuf path = STRBUF_INIT;
@@ -177,9 +179,9 @@ struct worktree **get_worktrees(unsigned flags)
 
list = xmalloc(alloc * sizeof(struct worktree *));
 
-   list[counter++] = get_main_worktree();
+   list[counter++] = get_main_worktree(git_common_dir);
 
-   strbuf_addf(, "%s/worktrees", get_git_common_dir());
+   strbuf_addf(, "%s/worktrees", git_common_dir);
dir = opendir(path.buf);
strbuf_release();
if (dir) {
@@ -188,7 +190,7 @@ struct worktree **get_worktrees(unsigned flags)
if (is_dot_or_dotdot(d->d_name))
continue;
 
-   if ((linked = get_linked_worktree(d->d_name))) {
+   if ((linked = get_linked_worktree(git_common_dir, 
d->d_name))) {
ALLOC_GROW(list, counter + 1, alloc);
list[counter++] = linked;
}
@@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
return list;
 }
 
+struct worktree **get_worktrees(unsigned flags)
+{
+   return get_worktrees_internal(get_git_common_dir(), flags);
+}
+
+struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
+{
+   char *submodule_gitdir;
+   struct strbuf sb = STRBUF_INIT;
+   struct worktree **ret;
+
+   submodule_gitdir = git_pathdup_submodule(path, "%s", "");
+   if (!submodule_gitdir)
+   return NULL;
+
+   /* the env would be set for the superproject */
+   get_common_dir_noenv(, submodule_gitdir);
+   ret = get_worktrees_internal(sb.buf, flags);
+
+   strbuf_release();
+   free(submodule_gitdir);
+   return ret;
+}
+
 const char *get_worktree_git_dir(const struct worktree *wt)
 {
if (!wt)
diff --git a/worktree.h b/worktree.h
index d59ce1fee8..157fbc4a66 100644
--- a/worktree.h
+++ b/worktree.h
@@ 

[PATCHv6 0/7] submodule embedgitdirs

[Stefan Beller]

v6:
 * renamed embedgitdirs to absorbgitdirs embedding may be interpreted as
   embedding the git dir into the working directory, whereas absorbing sounds
   more like the submodule is absorbed by the superproject, making the
   submodule less independent
 * Worktrees API offer uses_worktrees(void) and submodule_uses_worktree(path).
 * moved the printing to stderr and one layer up (out of the pure
   relocate_git_dir function).
 * connect_... is in dir.h now.

v5:
* Add another layer of abstraction, i.e. the relocate_git_dir is only about
  moving a git dir of one repository. The submodule specific stuff (e.g.
  recursion into nested submodules) is in submodule.{c,h}

  This was motivated by reviews on the series of checkout aware of submodules
  building on top of this series, as we want to directly call the embed-git-dirs
  function without the overhead of spawning a child process.

v4:
* rebuilt on top of nd/worktree-list-fixup
* fix and test behavior for un-init submodules (don't crash, rather do nothing)
* incorporated a "static" as pointed out by Ramsay
* use internal functions instead of duplicating code in worktree.c
  (use get_common_dir_noenv for the submodule to actually get the common dir)
* fixed a memory leak in relocate_gitdir

v3:
* have a slightly more generic function "relocate_gitdir".
  The recursion is strictly related to submodules, though.
* bail out if a submodule is using worktrees.
  This also lays the groundwork for later doing the proper thing,
  as worktree.h offers a function `get_submodule_worktrees(path)`
* nit by duy: use git_path instead of git_common_dir

v2:
* fixed commit message for patch:
 "submodule: use absolute path for computing relative path connecting"
* a new patch "submodule helper: support super prefix"
* redid the final patch with more tests and fixing bugs along the way
* "test-lib-functions.sh: teach test_commit -C " unchanged

v1:
The discussion of the submodule checkout series revealed to me that a command
is needed to move the git directory from the submodules working tree to be
embedded into the superprojects git directory.

So I wrote the code to intern the submodules git dir into the superproject,
but whilst writing the code I realized this could be valueable for our use
in testing too. So I exposed it via the submodule--helper. But as the
submodule helper ought to be just an internal API, we could also
offer it via the proper submodule command.

The command as it is has little value to the end user for now, but
breaking it out of the submodule checkout series hopefully makes review easier.

Thanks,
Stefan

Stefan Beller (7):
  submodule: use absolute path for computing relative path connecting
  submodule helper: support super prefix
  test-lib-functions.sh: teach test_commit -C 
  worktree: get worktrees from submodules
  worktree: add function to check if worktrees are in use
  move connect_work_tree_and_git_dir to dir.h
  submodule: add absorb-git-dir function

 Documentation/git-submodule.txt|  15 +
 builtin/submodule--helper.c|  69 
 dir.c  |  38 +++
 dir.h  |   4 ++
 git-submodule.sh   |   7 +-
 git.c  |   2 +-
 submodule.c| 127 ++---
 submodule.h|   5 +-
 t/t7412-submodule-absorbgitdirs.sh | 101 +
 t/test-lib-functions.sh|  20 --
 worktree.c |  70 +---
 worktree.h |  13 
 12 files changed, 418 insertions(+), 53 deletions(-)
 create mode 100755 t/t7412-submodule-absorbgitdirs.sh

-- 
2.11.0.rc2.30.gc512cbd.dirty

[PATCH] submodule--helper: set alternateLocation for cloned submodules

[vi0oss]

From: "Vitaly \"_Vi\" Shukela" 

In 31224cbdc7 (clone: recursive and reference option triggers
submodule alternates, 2016-08-17) a mechanism was added to
have submodules referenced.  It did not address _nested_
submodules, however.

This patch makes all not just the root repository, but also
all submodules (recursively) have submodule.alternateLocation
and submodule.alternateErrorStrategy configured, making Git
search for possible alternates for nested submodules as well.

As submodule's alternate target does not end in .git/objects
(rather .git/modules/qq/objects), this alternate target
path restriction for in add_possible_reference_from_superproject
relates from "*.git/objects" to just */objects".

New tests have been added to t7408-submodule-reference.

Signed-off-by: Vitaly _Vi Shukela 
---

Notes:
Third review: missing && in test fixed.

Mailmap change not included.

 builtin/submodule--helper.c| 19 ++--
 t/t7408-submodule-reference.sh | 66 ++
 2 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..92fd676 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(
 
/*
 * If the alternate object store is another repository, try the
-* standard layout with .git/modules//objects
+* standard layout with .git/(modules/)+/objects
 */
-   if (ends_with(alt->path, ".git/objects")) {
+   if (ends_with(alt->path, "/objects")) {
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -583,6 +583,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
struct string_list reference = STRING_LIST_INIT_NODUP;
+   char *sm_alternate = NULL, *error_strategy = NULL;
 
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", ,
@@ -672,6 +673,20 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
die(_("could not get submodule directory for '%s'"), path);
git_config_set_in_file(p, "core.worktree",
   relative_path(path, sm_gitdir, _path));
+
+   /* setup alternateLocation and alternateErrorStrategy in the cloned 
submodule if needed */
+   git_config_get_string("submodule.alternateLocation", _alternate);
+   if (sm_alternate)
+   git_config_set_in_file(p, "submodule.alternateLocation",
+  sm_alternate);
+   git_config_get_string("submodule.alternateErrorStrategy", 
_strategy);
+   if (error_strategy)
+   git_config_set_in_file(p, "submodule.alternateErrorStrategy",
+  error_strategy);
+
+   free(sm_alternate);
+   free(error_strategy);
+
strbuf_release();
strbuf_release(_path);
free(sm_gitdir);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1c1e289..e159fc5 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -125,4 +125,70 @@ test_expect_success 'ignoring missing submodule alternates 
passes clone and subm
)
 '
 
+test_expect_success 'preparing second superproject with a nested submodule 
plus partial clone' '
+   test_create_repo supersuper &&
+   (
+   cd supersuper &&
+   echo "I am super super." >file &&
+   git add file &&
+   git commit -m B-super-super-initial
+   git submodule add "file://$base_dir/super" subwithsub &&
+   git commit -m B-super-super-added &&
+   git submodule update --init --recursive &&
+   git repack -ad
+   ) &&
+   git clone supersuper supersuper2 &&
+   (
+   cd supersuper2 &&
+   git submodule update --init
+   )
+'
+
+# At this point there are three root-level positories: A, B, super and super2
+
+test_expect_success 'nested submodule alternate in works and is actually used' 
'
+   test_when_finished "rm -rf supersuper-clone" &&
+   git clone --recursive --reference supersuper supersuper 
supersuper-clone &&
+   (
+   cd supersuper-clone &&
+   # test superproject has alternates setup correctly
+   test_alternate_is_used .git/objects/info/alternates . &&
+   # immediate submodule has alternate:
+   test_alternate_is_used 
.git/modules/subwithsub/objects/info/alternates subwithsub &&
+   # nested submodule also has alternate:
+   test_alternate_is_used 

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 4:39 PM,   wrote:

>
> Previously test contained errorneous
> test_must_fail, which was masked by
> missing &&.

I wonder if we could make either
the test_must_fail intelligent to detect such a broken && call chain
or the test_expect_success macro to see for those broken chains.

Patch looks good to me except one very minor nit. :)

> +test_expect_success 'nested submodule alternate in works and is actually 
> used' '
> +   test_when_finished "rm -rf supersuper-clone" &&
> +   git clone --recursive --reference supersuper supersuper 
> supersuper-clone &&
> +   (
> +   cd supersuper-clone &&
> +   # test superproject has alternates setup correctly
> +   test_alternate_is_used .git/objects/info/alternates . &&
> +   # immediate submodule has alternate:
> +   test_alternate_is_used 
> .git/modules/subwithsub/objects/info/alternates subwithsub

here is a && missing ;)

Re: [PATCHv5 4/5] worktree: get worktrees from submodules

[Brandon Williams]

On 12/07, Stefan Beller wrote:
> On Wed, Dec 7, 2016 at 2:45 PM, Junio C Hamano  wrote:
> > Stefan Beller  writes:
> >
> >> + submodule_common_dir = strbuf_detach(, NULL);
> >> + ret = get_worktrees_internal(submodule_common_dir, flags);
> >> +
> >> + free(submodule_gitdir);
> >
> > This sequence felt somewhat unusual.  I would have written this
> > without an extra variable, i.e.
> >
> > ret = get_worktrees_internal(sb.buf, flags);
> > strbuf_release();
> 
> Yours is cleaner; I don't remember what I was thinking.
> 
> Feel free to squash it in; in case a resend is needed I will do that.

Just make sure to leave that free in as it refers to another variable
(submodule_gitdir).  It actually turns out there is a memory leak in the
original code because submodule_common_dir is never freed after being
detached from the strbuf.

-- 
Brandon Williams

Re: [PATCH 2/2] describe: add support for multiple match patterns

[Jacob Keller]

On Wed, Dec 7, 2016 at 4:20 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> Basically, this started as a script to try each pattern in sequence,
>> but this is slow, cumbersome and easy to mess up.
>>
>> You're suggesting just add a single second pattern that we will do
>> matches and discard any tag that matches that first?
>
> I am not suggesting anything. I was just trying to see how well what
> was designed and implemented supports the use case that motivated
> the feature. Think of it as a sanity check and review of the design.
>

Makes sense.

>> I think I can implement that pretty easily, and it should have simpler
>> semantics. We can discard first, and then match what remains easily.
>
> I actually think "multiple" and "negative" are orthogonal and both
> are good things.  If we are enhancing the filtering by refname
> patterns to allow multiple patterns (i.e. your patch), that is good,
> and it would be ideal if we can also have support for negative ones.

I can add support for negative matches pretty easily. I personally
don't see the value of "logical and" filters, ie to match only tags
that match all the given filters, though that does allow some other
forms of expression.

I do like the idea of negative filters, and I'll go ahead and work on
adding that as another extension.

Thanks,
Jake

Re: [PATCHv5 5/5] submodule: add embed-git-dir function

[Brandon Williams]

On 12/07, Stefan Beller wrote:
> + argv_array_pushl(, "--super-prefix", sb.buf,
> + "submodule--helper",
> +"embed-git-dirs", NULL);

check the spacing on these lines, looks like there is an extra space
before "submodule--helper"

-- 
Brandon Williams

[PATCH] submodule--helper: set alternateLocation for cloned submodules

[vi0oss]

From: "Vitaly \"_Vi\" Shukela" 

In 31224cbdc7 (clone: recursive and reference option triggers
submodule alternates, 2016-08-17) a mechanism was added to
have submodules referenced.  It did not address _nested_
submodules, however.

This patch makes all not just the root repository, but also
all submodules (recursively) have submodule.alternateLocation
and submodule.alternateErrorStrategy configured, making Git
search for possible alternates for nested submodules as well.

As submodule's alternate target does not end in .git/objects
(rather .git/modules/qq/objects), this alternate target
path restriction for in add_possible_reference_from_superproject
relates from "*.git/objects" to just */objects".

New tests have been added to t7408-submodule-reference.

Signed-off-by: Vitaly _Vi Shukela 
---

Notes:
Another round of fixups:

* Corrected code style
* Refactored and fixed the test

Previously test contained errorneous
test_must_fail, which was masked by
missing &&.

Mailmap patch omitted this time.

 builtin/submodule--helper.c| 19 ++--
 t/t7408-submodule-reference.sh | 66 ++
 2 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..92fd676 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(
 
/*
 * If the alternate object store is another repository, try the
-* standard layout with .git/modules//objects
+* standard layout with .git/(modules/)+/objects
 */
-   if (ends_with(alt->path, ".git/objects")) {
+   if (ends_with(alt->path, "/objects")) {
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -583,6 +583,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
struct string_list reference = STRING_LIST_INIT_NODUP;
+   char *sm_alternate = NULL, *error_strategy = NULL;
 
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", ,
@@ -672,6 +673,20 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
die(_("could not get submodule directory for '%s'"), path);
git_config_set_in_file(p, "core.worktree",
   relative_path(path, sm_gitdir, _path));
+
+   /* setup alternateLocation and alternateErrorStrategy in the cloned 
submodule if needed */
+   git_config_get_string("submodule.alternateLocation", _alternate);
+   if (sm_alternate)
+   git_config_set_in_file(p, "submodule.alternateLocation",
+  sm_alternate);
+   git_config_get_string("submodule.alternateErrorStrategy", 
_strategy);
+   if (error_strategy)
+   git_config_set_in_file(p, "submodule.alternateErrorStrategy",
+  error_strategy);
+
+   free(sm_alternate);
+   free(error_strategy);
+
strbuf_release();
strbuf_release(_path);
free(sm_gitdir);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1c1e289..9325297 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -125,4 +125,70 @@ test_expect_success 'ignoring missing submodule alternates 
passes clone and subm
)
 '
 
+test_expect_success 'preparing second superproject with a nested submodule 
plus partial clone' '
+   test_create_repo supersuper &&
+   (
+   cd supersuper &&
+   echo "I am super super." >file &&
+   git add file &&
+   git commit -m B-super-super-initial
+   git submodule add "file://$base_dir/super" subwithsub &&
+   git commit -m B-super-super-added &&
+   git submodule update --init --recursive &&
+   git repack -ad
+   ) &&
+   git clone supersuper supersuper2 &&
+   (
+   cd supersuper2 &&
+   git submodule update --init
+   )
+'
+
+# At this point there are three root-level positories: A, B, super and super2
+
+test_expect_success 'nested submodule alternate in works and is actually used' 
'
+   test_when_finished "rm -rf supersuper-clone" &&
+   git clone --recursive --reference supersuper supersuper 
supersuper-clone &&
+   (
+   cd supersuper-clone &&
+   # test superproject has alternates setup correctly
+   test_alternate_is_used .git/objects/info/alternates . &&
+   # immediate submodule has alternate:
+   test_alternate_is_used 
.git/modules/subwithsub/objects/info/alternates 

Re: [PATCH 01/17] mv: convert to using pathspec struct interface

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > Convert the 'internal_copy_pathspec()' function to use the pathspec
> > struct interface from using the deprecated 'get_pathspec()' interface.
> >
> > In addition to this, fix a memory leak caused by only duplicating some
> > of the pathspec elements.  Instead always duplicate all of the the
> > pathspec elements as an intermediate step (with modificationed based on
> > the passed in flags).  This way the intermediate strings can then be
> > freed prior to duplicating the result of parse_pathspec (which contains
> > each of the elements with the prefix prepended).
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  builtin/mv.c | 45 -
> >  1 file changed, 32 insertions(+), 13 deletions(-)
> 
> Stefan did something similar last year [1] but I couldn't find why it
> did not get merged. Not sure if the reasons are still relevant or
> not...
> 
> [1] 
> http://public-inbox.org/git/%3c1438885632-26470-1-git-send-email-sbel...@google.com%3E/
> 
> > diff --git a/builtin/mv.c b/builtin/mv.c
> > index 2f43877..4df4a12 100644
> > --- a/builtin/mv.c
> > +++ b/builtin/mv.c
> > @@ -4,6 +4,7 @@
> >   * Copyright (C) 2006 Johannes Schindelin
> >   */
> >  #include "builtin.h"
> > +#include "pathspec.h"
> >  #include "lockfile.h"
> >  #include "dir.h"
> >  #include "cache-tree.h"
> > @@ -25,25 +26,43 @@ static const char **internal_copy_pathspec(const char 
> > *prefix,
> >  {
> > int i;
> > const char **result;
> > +   struct pathspec ps;
> > ALLOC_ARRAY(result, count + 1);
> > -   COPY_ARRAY(result, pathspec, count);
> > -   result[count] = NULL;
> > +
> > +   /* Create an intermediate copy of the pathspec based on the flags */
> > for (i = 0; i < count; i++) {
> > -   int length = strlen(result[i]);
> > +   int length = strlen(pathspec[i]);
> > int to_copy = length;
> > +   char *it;
> > while (!(flags & KEEP_TRAILING_SLASH) &&
> > -  to_copy > 0 && is_dir_sep(result[i][to_copy - 1]))
> > +  to_copy > 0 && is_dir_sep(pathspec[i][to_copy - 1]))
> > to_copy--;
> > -   if (to_copy != length || flags & DUP_BASENAME) {
> > -   char *it = xmemdupz(result[i], to_copy);
> > -   if (flags & DUP_BASENAME) {
> > -   result[i] = xstrdup(basename(it));
> > -   free(it);
> > -   } else
> > -   result[i] = it;
> > -   }
> > +
> > +   it = xmemdupz(pathspec[i], to_copy);
> > +   if (flags & DUP_BASENAME) {
> > +   result[i] = xstrdup(basename(it));
> > +   free(it);
> > +   } else
> > +   result[i] = it;
> > +   }
> > +   result[count] = NULL;
> > +
> > +   parse_pathspec(,
> > +  PATHSPEC_ALL_MAGIC &
> > +  ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL),
> > +  PATHSPEC_KEEP_ORDER | PATHSPEC_PREFER_CWD,
> > +  prefix, result);
> > +   assert(count == ps.nr);
> > +
> > +   /* Copy the pathspec and free the old intermediate strings */
> > +   for (i = 0; i < count; i++) {
> > +   const char *match = xstrdup(ps.items[i].match);
> > +   free((char *) result[i]);
> > +   result[i] = match;
> 
> Sigh.. it looks so weird that we do all the parsing (in a _copy_
> pathspec function) then remove struct pathspec and return the plain
> string. I guess we can't do anything more until we rework cmd_mv code
> to handle pathspec natively.
> 
> At the least I think we should rename this function to something else.
> But if you have time I really wish we could kill this function. I
> haven't stared at cmd_mv() long and hard, but it looks to me that we
> combining two separate functionalities in the same function here.
> 
> If "mv" takes n arguments, then the first  arguments may be
> pathspec, the last one is always a plain path. The "dest_path =
> internal_copy_pathspec..." could be as simple as "dest_path =
> prefix_path(argv[argc - 1])". the special treatment for this last
> argument [1] can live here. Then, we can do parse_pathspec for the
>  arguments in cmd_mv(). It's still far from perfect, because
> cmd_mv can't handle pathspec properly, but it reduces the messy mess
> in internal_copy_pathspec a bit, I hope.
> 
> [1] c57f628 (mv: let 'git mv file no-such-dir/' error out - 2013-12-03)
> 
> > }
> > -   return get_pathspec(prefix, result);
> > +
> > +   clear_pathspec();
> > +   return result;
> >  }
> >
> >  static const char *add_slash(const char *path)
> > --
> > 2.8.0.rc3.226.g39d4020

Re: [PATCH 2/2] describe: add support for multiple match patterns

[Junio C Hamano]

Jacob Keller  writes:

> Basically, this started as a script to try each pattern in sequence,
> but this is slow, cumbersome and easy to mess up.
>
> You're suggesting just add a single second pattern that we will do
> matches and discard any tag that matches that first?

I am not suggesting anything. I was just trying to see how well what
was designed and implemented supports the use case that motivated
the feature. Think of it as a sanity check and review of the design.

> I think I can implement that pretty easily, and it should have simpler
> semantics. We can discard first, and then match what remains easily.

I actually think "multiple" and "negative" are orthogonal and both
are good things.  If we are enhancing the filtering by refname
patterns to allow multiple patterns (i.e. your patch), that is good,
and it would be ideal if we can also have support for negative ones.

Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > Convert 'create_simplify()' to use the pathspec struct interface from
> > using the '_raw' entry in the pathspec.
> 
> It would be even better to kill this create_simplify() and let
> simplify_away() handle struct pathspec directly.
> 
> There is a bug in this code, that might have been found if we
> simpify_away() handled pathspec directly: the memcmp() in
> simplify_away() will not play well with :(icase) magic. My bad. If
> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> maybe we can teach simplify_away() to do strncasecmp instead. We could
> ignore exclude patterns there too (although not excluding is not a
> bug).

So are you implying that the simplify struct needs to be killed?  That
way the pathspec struct itself is being passed around instead?

-- 
Brandon Williams

Re: [PATCH 1/2] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 2:49 PM,   wrote:
> Notes:
> Resolved issues pointed by Stefan Beller except of
> the one about loosened path check, which he aggreed
> to be relaxed for this case.

I am sorry to have given an incomplete review at the first time. :/
More below.

> +   /* setup alternateLocation and alternateErrorStrategy in the cloned 
> submodule if needed */
> +   git_config_get_string("submodule.alternateLocation", _alternate);
> +   if (sm_alternate) {
> +   git_config_set_in_file(p, "submodule.alternateLocation",
> +  sm_alternate);
> +   }

For a single command, we usually omit the braces for
an if clause, i.e.

if (foo)
bar(...);

/* code goes on */


> +   git_config_get_string("submodule.alternateErrorStrategy", 
> _strategy);
> +   if (error_strategy) {
> +   git_config_set_in_file(p, "submodule.alternateErrorStrategy",
> +  error_strategy);
> +   }
> +
> +   free(sm_alternate);
> +   free(error_strategy);
> +

The indentation seems a bit off for the free here?
(The main nit that motivated me writing the email)


> strbuf_release();
> strbuf_release(_path);
> free(sm_gitdir);
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index 1c1e289..ef7771b 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -125,4 +125,76 @@ test_expect_success 'ignoring missing submodule 
> alternates passes clone and subm
> )
>  '
>
> +test_expect_success 'preparing second superproject with a nested submodule' '
> +   test_create_repo supersuper &&
> +   (
> +   cd supersuper &&
> +   echo "I am super super." >file &&
> +   git add file &&
> +   git commit -m B-super-super-initial
> +   git submodule add "file://$base_dir/super" subwithsub &&
> +   git commit -m B-super-super-added &&
> +   git submodule update --init --recursive &&
> +   git repack -ad
> +   )
> +'
> +
> +# At this point there are three root-level positories: A, B, super and super2
> +
> +test_expect_success 'nested submodule alternate in works and is actually 
> used' '
> +   test_when_finished "rm -rf supersuper-clone" &&
> +   git clone --recursive --reference supersuper supersuper 
> supersuper-clone &&
> +   (
> +   cd supersuper-clone &&
> +   # test superproject has alternates setup correctly
> +   test_alternate_is_used .git/objects/info/alternates . &&
> +   # immediate submodule has alternate:
> +   test_alternate_is_used 
> .git/modules/subwithsub/objects/info/alternates subwithsub
> +   # nested submodule also has alternate:
> +   test_alternate_is_used 
> .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> +   )
> +'
> +
> +test_expect_success 'missing nested submodule alternate fails clone and 
> submodule update' '
> +   test_when_finished "rm -rf supersuper-clone supersuper2" &&
> +   git clone supersuper supersuper2 &&
> +   (
> +   cd supersuper2 &&
> +   git submodule update --init
> +   ) &&
> +   test_must_fail git clone --recursive --reference supersuper2 
> supersuper2 supersuper-clone &&
> +   (
> +   cd supersuper-clone &&
> +   # test superproject has alternates setup correctly
> +   test_alternate_is_used .git/objects/info/alternates . &&
> +   # update of the submodule fails
> +   test_must_fail git submodule update --init --recursive &&
> +   # immediate submodule has alternate:
> +   test_alternate_is_used 
> .git/modules/subwithsub/objects/info/alternates subwithsub
> +   # but nested submodule has no alternate:
> +   test_must_fail test_alternate_is_used 
> .git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
> +   )
> +'
> +
> +test_expect_success 'missing nested submodule alternate in 
> --reference-if-able mode' '
> +   test_when_finished "rm -rf supersuper-clone supersuper2" &&
> +   git clone supersuper supersuper2 &&
> +   (
> +   cd supersuper2 &&
> +   git submodule update --init
> +   ) &&
> +   git clone --recursive --reference-if-able supersuper2 supersuper2 
> supersuper-clone &&
> +   (
> +   cd supersuper-clone &&
> +   # test superproject has alternates setup correctly
> +   test_alternate_is_used .git/objects/info/alternates . &&
> +   # update of the submodule fails
> +   test_must_fail git submodule update --init --recursive &&
> +   # immediate submodule has alternate:
> + 

Re: [PATCH v8 00/19] port branch.c to use ref-filter's printing options

[Jacob Keller]

On Wed, Dec 7, 2016 at 7:36 AM, Karthik Nayak  wrote:
> This is part of unification of the commands 'git tag -l, git branch -l
> and git for-each-ref'. This ports over branch.c to use ref-filter's
> printing options.
>
> Initially posted here: $(gmane/279226). It was decided that this series
> would follow up after refactoring ref-filter parsing mechanism, which
> is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>
> v1 can be found here: $(gmane/288342)
> v2 can be found here: $(gmane/288863)
> v3 can be found here: $(gmane/290299)
> v4 can be found here: $(gmane/291106)
> v5b can be found here: $(gmane/292467)
> v6 can be found here: http://marc.info/?l=git=146330914118766=2
> v7 can be found here: http://marc.info/?l=git=147863593317362=2
>
> Changes in this version:
>
> 1. use an enum for holding the comparision type in
> %(if:[equals/notequals=...]) options.
> 2. rename the 'strip' option to 'lstrip' and introduce an 'rstrip'
> option. Also modify them to take negative values. This drops the
> ':dri' and ':base' options.
> 3. Drop unecessary code.
> 4. Cleanup code and fix spacing.
> 5. Add more comments wherever required.
> 6. Add quote_literal_for_format(const char *s) for safer string
> insertions in branch.c:build_format().
>
> Thanks to Jacob, Jackub, Junio and Matthieu for their inputs on the
> previous version.
>
> Interdiff below.
>
> Karthik Nayak (19):
>   ref-filter: implement %(if), %(then), and %(else) atoms
>   ref-filter: include reference to 'used_atom' within 'atom_value'
>   ref-filter: implement %(if:equals=) and
> %(if:notequals=)
>   ref-filter: modify "%(objectname:short)" to take length
>   ref-filter: move get_head_description() from branch.c
>   ref-filter: introduce format_ref_array_item()
>   ref-filter: make %(upstream:track) prints "[gone]" for invalid
> upstreams
>   ref-filter: add support for %(upstream:track,nobracket)
>   ref-filter: make "%(symref)" atom work with the ':short' modifier
>   ref-filter: introduce refname_atom_parser_internal()
>   ref-filter: introduce refname_atom_parser()
>   ref-filter: make remote_ref_atom_parser() use
> refname_atom_parser_internal()
>   ref-filter: rename the 'strip' option to 'lstrip'
>   ref-filter: modify the 'lstrip=' option to work with negative ''
>   ref-filter: add an 'rstrip=' option to atoms which deal with
> refnames
>   ref-filter: allow porcelain to translate messages in the output
>   branch, tag: use porcelain output
>   branch: use ref-filter printing APIs
>   branch: implement '--format' option
>
>  Documentation/git-branch.txt   |   7 +-
>  Documentation/git-for-each-ref.txt |  86 +--
>  builtin/branch.c   | 290 +++---
>  builtin/tag.c  |   6 +-
>  ref-filter.c   | 488 
> +++--
>  ref-filter.h   |   7 +
>  t/t3203-branch-output.sh   |  16 +-
>  t/t6040-tracking-info.sh   |   2 +-
>  t/t6300-for-each-ref.sh|  88 ++-
>  t/t6302-for-each-ref-filter.sh |  94 +++
>  10 files changed, 784 insertions(+), 300 deletions(-)
>
> Interdiff:
>
> diff --git a/Documentation/git-for-each-ref.txt 
> b/Documentation/git-for-each-ref.txt
> index f4ad297..c72baeb 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -92,13 +92,14 @@ refname::
> The name of the ref (the part after $GIT_DIR/).
> For a non-ambiguous short name of the ref append `:short`.
> The option core.warnAmbiguousRefs is used to select the strict
> -   abbreviation mode. If `strip=` is appended, strips ``
> -   slash-separated path components from the front of the refname
> -   (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
> -   `` must be a positive integer.  If a displayed ref has fewer
> -   components than ``, the command aborts with an error. For the base
> -   directory of the ref (i.e. foo in refs/foo/bar/boz) append
> -   `:base`. For the entire directory path append `:dir`.
> +   abbreviation mode. If `lstrip=` or `rstrip=` option can

Grammar here, drop the If before `lstrip since you're referring to
multiples and you say "x can be appended to y" rather than "if x is
added, do y"

> +   be appended to strip `` slash-separated path components
> +   from or end of the refname respectively (e.g.,
> +   `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo` and
> +   `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`).  if
> +   `` is a negative number, then only `` path components
> +   are left behind.  If a displayed ref has fewer components than
> +   ``, the command aborts with an error.
>

Would it make more sense to not die and instead just return the empty
string? On the one hand, if we die() it's obvious that you tried to
strip too many components. But on the other hand, 

Re: [PATCH 2/2] describe: add support for multiple match patterns

[Jacob Keller]

On Wed, Dec 7, 2016 at 2:08 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> ... Suppose that you version all
>> your official releases such as "v1.2", "v1.3", "v1.4", "v2.1" and so on.
>> Now, you also have other tags which represent -rc releases and other
>> such tags. If you want to find the first major release that contains
>> a given commit you might try
>>
>> git describe --contains --match="v?.?" 
>>
>> This will work as long as you have only single digits. But if you start
>> adding multiple digits, the pattern becomes not enough to match all the
>> tags you wanted while excluding the ones you didn't.
>
> Isn't what you really want for the use case a negative pattern,
> i.e. "I want ones that match v* but not the ones that match *-rc*",
> though?

That's another way of implementing it. I just went with straight
forward patterns that I was already using in sequence.

Basically, this started as a script to try each pattern in sequence,
but this is slow, cumbersome and easy to mess up.

You're suggesting just add a single second pattern that we will do
matches and discard any tag that matches that first?

I think I can implement that pretty easily, and it should have simpler
semantics. We can discard first, and then match what remains easily.

Thanks,
Jake

Re: [PATCHv5 0/5] submodule embedgitdirs

[Stefan Beller]

On Wed, Dec 7, 2016 at 3:34 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Stefan Beller  writes:
>>
>>> v5:
>>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about
>>>   moving a git dir of one repository. The submodule specific stuff (e.g.
>>>   recursion into nested submodules) is in submodule.{c,h}
>>>
>>>   This was motivated by reviews on the series of checkout aware of 
>>> submodules
>>>   building on top of this series, as we want to directly call the 
>>> embed-git-dirs
>>>   function without the overhead of spawning a child process.
>>
>> OK.  Comparing the last steps between this round and the previous
>> one, I do think the separation of the responsibility among helpers
>> is much more reasonable in this version, where:
>>
>>  - submodule_embed_git_dir() is given a single path and is
>>responsible for that submodule itself, which is done by calling
>>submodule_embed_git_dir_for_path() on itself, and its
>>sub-submodules, which is done by spawning the helper recursively
>>with appropriate super-prefix;
>>
>>  - submodule_embed_git_dir_for_path() computes where the given path
>>needs to be moved to using the knowledge specific to the
>>submodule subsystem, and asks relocate_gitdir() to perform the
>>actual relocation;
>>
>>  - relocate_gitdir() used to do quite a lot more, but now it is only
>>about moving an existing .git directory elsewhere and pointing to
>>the new location with .git file placed in the old location.
>>
>> I would have called the second helper submodule_embed_one_git_dir(),
>> but that is a minor detail.
>>
>> Very nicely done.
>
> One thing that is not so nice from the code organization point of
> view is that "dir.c" now needs to include "submodule.h" because it
> wants to call connect_work_tree_and_git_dir().
>
> I wonder if that function belongs to dir.c or worktree.c not
> submodule.c; I do not see anything that is submodule specific about
> what the function does.

Right, it just happened historically for that function to live
in submodule area. I'll move the connect_work_tree_and_git_dir
to dir.c as well.

Re: [PATCH v2 0/6] shallow.c improvements

[Junio C Hamano]

Duy Nguyen  writes:

> On Tue, Dec 6, 2016 at 8:42 PM, Jeff King  wrote:
>> The final one _seems_ reasonable after reading your explanation, but I
>> lack enough context to know whether or not there might be a corner case
>> that you're missing. I'm inclined to trust your assessment on it.
>
> Yeah I basically just wrote down my thoughts so somebody could maybe
> spot something wrong. I'm going to think about it some more in the
> next few days.

In the meantime let me queue them as-is.

Thanks.

Re: [PATCHv5 5/5] submodule: add embed-git-dir function

[Stefan Beller]

On Wed, Dec 7, 2016 at 3:03 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> @@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {
>>   {"resolve-relative-url", resolve_relative_url, 0},
>>   {"resolve-relative-url-test", resolve_relative_url_test, 0},
>>   {"init", module_init, 0},
>> - {"remote-branch", resolve_remote_submodule_branch, 0}
>> + {"remote-branch", resolve_remote_submodule_branch, 0},
>> + {"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
>>  };
>
> If you want to avoid patch noise like this, your 2/5 can add a
> trailing comma after the entry for remote-branch.  It is OK to end
> elements in an array literal with trailing comma, even though we
> avoid doing similar in enum definition (which is only allowed in
> newer C standards).

Ok, thanks for pointing out!

>
>> diff --git a/dir.c b/dir.c
>> index bfa8c8a9a5..e023b04407 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state 
>> *istate,
>>  {
>>   untracked_cache_invalidate_path(istate, path);
>>  }
>> +
>> +/*
>> + * Migrate the git directory of the given `path` from `old_git_dir` to
>> + * `new_git_dir`. If an error occurs, append it to `err` and return the
>> + * error code.
>> + */
>> +int relocate_gitdir(const char *path, const char *old_git_dir,
>> + const char *new_git_dir, const char *displaypath,
>> + struct strbuf *err)
>> +{
>> + int ret = 0;
>> +
>> + printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
>> + displaypath, old_git_dir, new_git_dir);
>
> By using "strbuf err", it looks like that the calling convention of
> this function wants to cater to callers who want to have tight
> control over their output and an unconditional printing to the
> standard output looks somewhat out of place.

Before sending it out to the list I had a version with another flag
that controlled whether we die on error or just print a warning.
That was not fully true however as the connect_work_tree_and_git_dir
would die nevertheless, we'd only warn for the failed rename().

It turns out we do not need a fully functional non-die()ing version
for the checkout series, so I ripped that out again and the version sent out
just dies in case of failure in relocate_gitdir.

That said, I think the printing of the migration message ought to go
into the caller and to stderr as you note.

>
> Besides, does it belong to the standard output?  It looks more like
> a progress-bar eye candy that we send out to the standard error
> stream (and only when we are talking to a tty).
>
>> +/* Embeds a single submodule, non recursively. */
>> +static void submodule_embed_git_dir_for_path(const char *prefix, const char 
>> *path)
>> +{
>> + struct worktree **worktrees;
>> + struct strbuf pathbuf = STRBUF_INIT;
>> + struct strbuf errbuf = STRBUF_INIT;
>> + char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir 
>> = NULL;
>> + const char *new_git_dir;
>> + const struct submodule *sub;
>> + int code;
>> +
>> + worktrees = get_submodule_worktrees(path, 0);
>> + if (worktrees) {
>> + int i;
>> + for (i = 0; worktrees[i]; i++)
>> + ;
>> + free_worktrees(worktrees);
>> + if (i > 1)
>> + die(_("relocate_gitdir for submodule '%s' with "
>> + "more than one worktree not supported"), path);
>> + }
>
> We may benefit from "does this have secondary worktrees?" boolean
> helper function, perhaps?

ok, I'll add that helper as its own patch to the worktree code earlier
in the series.

>
>> + old_git_dir = xstrfmt("%s/.git", path);
>> + if (read_gitfile(old_git_dir))
>> + /* If it is an actual gitfile, it doesn't need migration. */
>> + return;
>
> Isn't this "no-op return" a valid thing to do, even when the
> submodule has secondary worktrees that borrow from it?

If we have 2 worktrees, all bets are off (in my non-understanding).
The git file here *could* point to git directory living inside the
other working tree, which then would need migration from that other
working tree to some embedded place.

I don't think that is how worktrees work (or have ever worked?)
but I am unfamiliar with worktrees, so I erred on the safe side.

>  I am
> wondering if the "ah, we don't do a repository that has secondary
> worktrees" check should come after this one.

Maybe Duy can answer that? (Where are git directories
located in a worktree setup, even historically? Were they
ever inside one of the submodules? The other working tree may
not be a submodule, but a standalone thing as well?)

>
>> + real_old_git_dir = xstrdup(real_path(old_git_dir));
>> +...
>> +}
>
>> +/*
>> + * Migrate the git directory of the submodule given by path from
>> + * having its git directory within the working tree 

Re: [PATCHv5 0/5] submodule embedgitdirs

[Junio C Hamano]

Junio C Hamano  writes:

> Stefan Beller  writes:
>
>> v5:
>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about 
>>   moving a git dir of one repository. The submodule specific stuff (e.g.
>>   recursion into nested submodules) is in submodule.{c,h}
>>   
>>   This was motivated by reviews on the series of checkout aware of submodules
>>   building on top of this series, as we want to directly call the 
>> embed-git-dirs
>>   function without the overhead of spawning a child process.
>
> OK.  Comparing the last steps between this round and the previous
> one, I do think the separation of the responsibility among helpers
> is much more reasonable in this version, where:
>
>  - submodule_embed_git_dir() is given a single path and is
>responsible for that submodule itself, which is done by calling
>submodule_embed_git_dir_for_path() on itself, and its
>sub-submodules, which is done by spawning the helper recursively
>with appropriate super-prefix;
>
>  - submodule_embed_git_dir_for_path() computes where the given path
>needs to be moved to using the knowledge specific to the
>submodule subsystem, and asks relocate_gitdir() to perform the
>actual relocation;
>
>  - relocate_gitdir() used to do quite a lot more, but now it is only
>about moving an existing .git directory elsewhere and pointing to
>the new location with .git file placed in the old location.
>
> I would have called the second helper submodule_embed_one_git_dir(),
> but that is a minor detail.
>
> Very nicely done.

One thing that is not so nice from the code organization point of
view is that "dir.c" now needs to include "submodule.h" because it
wants to call connect_work_tree_and_git_dir().

I wonder if that function belongs to dir.c or worktree.c not
submodule.c; I do not see anything that is submodule specific about
what the function does.

Re: [PATCH 16/17] pathspec: small readability changes

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > A few small changes to improve readability.  This is done by grouping 
> > related
> > assignments, adding blank lines, ensuring lines are <80 characters, etc.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  pathspec.c | 15 ++-
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 41aa213..8a07b02 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> > if ((magic & PATHSPEC_LITERAL) && (magic & PATHSPEC_GLOB))
> > die(_("%s: 'literal' and 'glob' are incompatible"), elt);
> >
> > +   /* Create match string which will be used for pathspec matching */
> > if (pathspec_prefix >= 0) {
> > match = xstrdup(copyfrom);
> > prefixlen = pathspec_prefix;
> > @@ -341,11 +342,16 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> > match = xstrdup(copyfrom);
> > prefixlen = 0;
> > } else {
> > -   match = prefix_path_gently(prefix, prefixlen, , 
> > copyfrom);
> > +   match = prefix_path_gently(prefix, prefixlen,
> > +  , copyfrom);
> > if (!match)
> > die(_("%s: '%s' is outside repository"), elt, 
> > copyfrom);
> > }
> > +
> > item->match = match;
> > +   item->len = strlen(item->match);
> > +   item->prefix = prefixlen;
> > +
> > /*
> >  * Prefix the pathspec (keep all magic) and assign to
> >  * original. Useful for passing to another command.
> > @@ -362,8 +368,6 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> > } else {
> > item->original = xstrdup(elt);
> > }
> > -   item->len = strlen(item->match);
> > -   item->prefix = prefixlen;
> >
> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
> > strip_submodule_slash_cheap(item);
> > @@ -371,13 +375,14 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> > if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> > strip_submodule_slash_expensive(item);
> >
> > -   if (magic & PATHSPEC_LITERAL)
> > +   if (magic & PATHSPEC_LITERAL) {
> > item->nowildcard_len = item->len;
> > -   else {
> > +   } else {
> > item->nowildcard_len = simple_length(item->match);
> > if (item->nowildcard_len < prefixlen)
> > item->nowildcard_len = prefixlen;
> > }
> > +
> > item->flags = 0;
> 
> You probably can move this line up with the others too.

I didn't move the item->flags assignment up since the code immediately
following this assignment deal with setting item->flags.  I made more
sense to keep them grouped.

-- 
Brandon Williams

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Duy Nguyen]

On Thu, Dec 8, 2016 at 3:35 AM, Stephan Beyer  wrote:
> Hi,
>
> On 12/07/2016 09:04 PM, Junio C Hamano wrote:
>> Stephan Beyer  writes:
>>
>>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>>> different wording for the same thing makes things unintuitive.
>>
>> It is not too late to STOP "--forget" from getting added to "rebase"
>> and give it a better name.

Sorry I didn't know about --quit (and it has been there since 2011, I
guess I'm just not big sequencer user).

> Oh. ;) I am not sure. I personally think that --forget is a better name

Yeah, I was stuck with the name --destroy for many months and was very
happy the day I found --forget, which does not imply any destructive
side effects and is distinct enough from --abort to not confuse
people.

> than --quit because when I hear --quit I tend to look into the manual
> page first to check if there are weird side effects (and then the manual
> page says that it "forgets" ;D).
> So I'd rather favor adding --forget to cherry-pick/revert instead... or
> this:
-- 
Duy

Re: [PATCHv5 5/5] submodule: add embed-git-dir function

[Junio C Hamano]

Stefan Beller  writes:

> @@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {
>   {"resolve-relative-url", resolve_relative_url, 0},
>   {"resolve-relative-url-test", resolve_relative_url_test, 0},
>   {"init", module_init, 0},
> - {"remote-branch", resolve_remote_submodule_branch, 0}
> + {"remote-branch", resolve_remote_submodule_branch, 0},
> + {"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
>  };

If you want to avoid patch noise like this, your 2/5 can add a
trailing comma after the entry for remote-branch.  It is OK to end
elements in an array literal with trailing comma, even though we
avoid doing similar in enum definition (which is only allowed in
newer C standards).

> diff --git a/dir.c b/dir.c
> index bfa8c8a9a5..e023b04407 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state 
> *istate,
>  {
>   untracked_cache_invalidate_path(istate, path);
>  }
> +
> +/*
> + * Migrate the git directory of the given `path` from `old_git_dir` to
> + * `new_git_dir`. If an error occurs, append it to `err` and return the
> + * error code.
> + */
> +int relocate_gitdir(const char *path, const char *old_git_dir,
> + const char *new_git_dir, const char *displaypath,
> + struct strbuf *err)
> +{
> + int ret = 0;
> +
> + printf("Migrating git directory of '%s' from\n'%s' to\n'%s'\n",
> + displaypath, old_git_dir, new_git_dir);

By using "strbuf err", it looks like that the calling convention of
this function wants to cater to callers who want to have tight
control over their output and an unconditional printing to the
standard output looks somewhat out of place.

Besides, does it belong to the standard output?  It looks more like
a progress-bar eye candy that we send out to the standard error
stream (and only when we are talking to a tty).

> +/* Embeds a single submodule, non recursively. */
> +static void submodule_embed_git_dir_for_path(const char *prefix, const char 
> *path)
> +{
> + struct worktree **worktrees;
> + struct strbuf pathbuf = STRBUF_INIT;
> + struct strbuf errbuf = STRBUF_INIT;
> + char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = 
> NULL;
> + const char *new_git_dir;
> + const struct submodule *sub;
> + int code;
> +
> + worktrees = get_submodule_worktrees(path, 0);
> + if (worktrees) {
> + int i;
> + for (i = 0; worktrees[i]; i++)
> + ;
> + free_worktrees(worktrees);
> + if (i > 1)
> + die(_("relocate_gitdir for submodule '%s' with "
> + "more than one worktree not supported"), path);
> + }

We may benefit from "does this have secondary worktrees?" boolean
helper function, perhaps?

> + old_git_dir = xstrfmt("%s/.git", path);
> + if (read_gitfile(old_git_dir))
> + /* If it is an actual gitfile, it doesn't need migration. */
> + return;

Isn't this "no-op return" a valid thing to do, even when the
submodule has secondary worktrees that borrow from it?  I am
wondering if the "ah, we don't do a repository that has secondary
worktrees" check should come after this one.

> + real_old_git_dir = xstrdup(real_path(old_git_dir));
> +...
> +}

> +/*
> + * Migrate the git directory of the submodule given by path from
> + * having its git directory within the working tree to the git dir nested
> + * in its superprojects git dir under modules/.
> + */

I think that this operation removes the git directories that are
embedded in the working tree of the superproject and storing them
away to safer place, i.e. unembedding.

> +int submodule_embed_git_dir(const char *prefix,
> + const char *path,
> + unsigned flags)
> +{
> + const char *sub_git_dir, *v;
> + char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
> + struct strbuf gitdir = STRBUF_INIT;
> +
> +

Lose the extra blank line here?

> + strbuf_addf(, "%s/.git", path);
> + sub_git_dir = resolve_gitdir(gitdir.buf);
> +
> + /* Not populated? */
> + if (!sub_git_dir)
> + goto out;
> +
> + /* Is it already embedded? */
> + real_sub_git_dir = xstrdup(real_path(sub_git_dir));
> + real_common_git_dir = xstrdup(real_path(get_git_common_dir()));
> + if (!skip_prefix(real_sub_git_dir, real_common_git_dir, ))

Yeah, checking for NULL-ness with !skip_prefix() helps ;-)

> + submodule_embed_git_dir_for_path(prefix, path);
> +
> + if (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + if (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)
> + die("BUG: we don't know how to pass the flags down?");
> +
> + if 

Re: [PATCHv5 4/5] worktree: get worktrees from submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 2:45 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> + submodule_common_dir = strbuf_detach(, NULL);
>> + ret = get_worktrees_internal(submodule_common_dir, flags);
>> +
>> + free(submodule_gitdir);
>
> This sequence felt somewhat unusual.  I would have written this
> without an extra variable, i.e.
>
> ret = get_worktrees_internal(sb.buf, flags);
> strbuf_release();

Yours is cleaner; I don't remember what I was thinking.

Feel free to squash it in; in case a resend is needed I will do that.

Thanks,
Stefan

[PATCH 2/2] mailmap: Update my e-mail address

[vi0oss]

From: "Vitaly \"_Vi\" Shukela" 

Signed-off-by: Vitaly "_Vi" Shukela 
---
 .mailmap | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.mailmap b/.mailmap
index 9cc33e9..b7ae81a 100644
--- a/.mailmap
+++ b/.mailmap
@@ -246,7 +246,7 @@ Uwe Kleine-König  
 
 Uwe Kleine-König  

 Ville Skyttä  
-Vitaly "_Vi" Shukela 
+Vitaly "_Vi" Shukela  
 W. Trevor King  
 William Pursell 
 YONETANI Tomokazu  
-- 
2.10.2

[PATCH 1/2] submodule--helper: set alternateLocation for cloned submodules

[vi0oss]

From: "Vitaly \"_Vi\" Shukela" 

In 31224cbdc7 (clone: recursive and reference option triggers
submodule alternates, 2016-08-17) a mechanism was added to
have submodules referenced.  It did not address _nested_
submodules, however.

This patch makes all not just the root repository, but also
all submodules (recursively) have submodule.alternateLocation
and submodule.alternateErrorStrategy configured, making Git
search for possible alternates for nested submodules as well.

As submodule's alternate target does not end in .git/objects
(rather .git/modules/qq/objects), this alternate target
path restriction for in add_possible_reference_from_superproject
relates from "*.git/objects" to just */objects".

New tests have been added to t7408-submodule-reference.

Signed-off-by: Vitaly _Vi Shukela 
---

Notes:
Resolved issues pointed by Stefan Beller except of
the one about loosened path check, which he aggreed
to be relaxed for this case.

 builtin/submodule--helper.c| 21 ++--
 t/t7408-submodule-reference.sh | 72 ++
 2 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..7b7633d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(
 
/*
 * If the alternate object store is another repository, try the
-* standard layout with .git/modules//objects
+* standard layout with .git/(modules/)+/objects
 */
-   if (ends_with(alt->path, ".git/objects")) {
+   if (ends_with(alt->path, "/objects")) {
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -583,6 +583,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
struct string_list reference = STRING_LIST_INIT_NODUP;
+   char *sm_alternate = NULL, *error_strategy = NULL;
 
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", ,
@@ -672,6 +673,22 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
die(_("could not get submodule directory for '%s'"), path);
git_config_set_in_file(p, "core.worktree",
   relative_path(path, sm_gitdir, _path));
+
+   /* setup alternateLocation and alternateErrorStrategy in the cloned 
submodule if needed */
+   git_config_get_string("submodule.alternateLocation", _alternate);
+   if (sm_alternate) {
+   git_config_set_in_file(p, "submodule.alternateLocation",
+  sm_alternate);
+   }
+   git_config_get_string("submodule.alternateErrorStrategy", 
_strategy);
+   if (error_strategy) {
+   git_config_set_in_file(p, "submodule.alternateErrorStrategy",
+  error_strategy);
+   }
+
+   free(sm_alternate);
+   free(error_strategy);
+
strbuf_release();
strbuf_release(_path);
free(sm_gitdir);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1c1e289..ef7771b 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -125,4 +125,76 @@ test_expect_success 'ignoring missing submodule alternates 
passes clone and subm
)
 '
 
+test_expect_success 'preparing second superproject with a nested submodule' '
+   test_create_repo supersuper &&
+   (
+   cd supersuper &&
+   echo "I am super super." >file &&
+   git add file &&
+   git commit -m B-super-super-initial
+   git submodule add "file://$base_dir/super" subwithsub &&
+   git commit -m B-super-super-added &&
+   git submodule update --init --recursive &&
+   git repack -ad
+   )
+'
+
+# At this point there are three root-level positories: A, B, super and super2
+
+test_expect_success 'nested submodule alternate in works and is actually used' 
'
+   test_when_finished "rm -rf supersuper-clone" &&
+   git clone --recursive --reference supersuper supersuper 
supersuper-clone &&
+   (
+   cd supersuper-clone &&
+   # test superproject has alternates setup correctly
+   test_alternate_is_used .git/objects/info/alternates . &&
+   # immediate submodule has alternate:
+   test_alternate_is_used 
.git/modules/subwithsub/objects/info/alternates subwithsub
+   # nested submodule also has alternate:
+   test_alternate_is_used 
.git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+   )
+'
+
+test_expect_success 

Re: [PATCH 03/17] dir: convert fill_directory to use the pathspec struct interface

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > Convert 'fill_directory()' to use the pathspec struct interface from
> > using the '_raw' entry in the pathspec struct.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  dir.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/dir.c b/dir.c
> > index 7df292b..8730a4f 100644
> > --- a/dir.c
> > +++ b/dir.c
> > @@ -188,7 +188,8 @@ int fill_directory(struct dir_struct *dir, const struct 
> > pathspec *pathspec)
> > len = common_prefix_len(pathspec);
> >
> > /* Read the directory and prune it */
> > -   read_directory(dir, pathspec->nr ? pathspec->_raw[0] : "", len, 
> > pathspec);
> > +   read_directory(dir, pathspec->nr ? pathspec->items[0].match : "",
> > +  len, pathspec);
> 
> Or even better, use common_prefix()'s return value here. I took me a
> while to realize this code was not buggy. It is fine to just pick the
> first item because the first  characters of _all_ pathspec items
> must be the same. Something like this
> 
> prefix = common_prefix(..)
> read_directory(..., prefix, strlen(prefix), pathspec);
> 
> expresses it much better. Yeah one extra mem allocation, no big deal
> since fill_directory() is not called very often.

I didn't even notice that.  Now looking at this you're right that its
not immediately obvious that what's there is correct.  I'll change this.

> 
> > return len;
> >  }
> >
> > --
> > 2.8.0.rc3.226.g39d4020
> >
> -- 
> Duy

-- 
Brandon Williams

Re: [PATCHv5 4/5] worktree: get worktrees from submodules

[Junio C Hamano]

Stefan Beller  writes:

> + submodule_common_dir = strbuf_detach(, NULL);
> + ret = get_worktrees_internal(submodule_common_dir, flags);
> +
> + free(submodule_gitdir);

This sequence felt somewhat unusual.  I would have written this
without an extra variable, i.e.

ret = get_worktrees_internal(sb.buf, flags);
strbuf_release();

Re: [PATCH 04/17] ls-tree: convert show_recursive to use the pathspec struct interface

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > Convert 'show_recursive()' to use the pathspec struct interface from
> > using the '_raw' entry in the pathspec struct.
> 
> Slightly off-topic (sorry, but you made me look at this code! :D),
> could you update the magic_mask argument of parse_pathspec() in this
> file to PATHSPEC_ALL_MAGIC & ~(PATHSPEC_FROMTOP | PATHSPEC_LITERAL)?
> It makes sure all future magic will be caught as unsupported (and I
> think Stefan is adding one, but understandably he did not find this
> code).
> 
> I think it's in the spirit of renaming _raw to match too. By limiting
> magic to fromtop and literal, we are sure match can only be path and
> nothing else, which is good because this show_recursive can't handle
> anything else either.

Can do.

-- 
Brandon Williams

Re: [PATCH 09/17] pathspec: always show mnemonic and name in unsupported_magic

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > @@ -413,10 +411,9 @@ void parse_pathspec(struct pathspec *pathspec,
> > prefixlen = prefix ? strlen(prefix) : 0;
> >
> > for (i = 0; i < n; i++) {
> > -   unsigned short_magic;
> > entry = argv[i];
> >
> > -   item[i].magic = prefix_pathspec(item + i, _magic,
> > +   item[i].magic = prefix_pathspec(item + i,
> > flags,
> > prefix, prefixlen, entry);
> 
> The final output looks a bit ...um.. strangely tall, with the first
> two lines that have one argument each, then the last line comes with
> three arguments. Maybe put 'flags' in the same line as 'item + i'?

Yep you're right, it does look a bit funny.

-- 
Brandon Williams

Re: [PATCH 09/17] pathspec: always show mnemonic and name in unsupported_magic

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > @@ -426,8 +423,7 @@ void parse_pathspec(struct pathspec *pathspec,
> > nr_exclude++;
> > if (item[i].magic & magic_mask)
> > unsupported_magic(entry,
> > - item[i].magic & magic_mask,
> > - short_magic);
> > + item[i].magic & magic_mask);
> 
> Same here. Maybe put both arguments in the same line. It looks a bit
> better. (sorry for two mails on the same patch, I'm reading the final
> output first before going through individual patches that breaks this
> function down)

All good.  Sometimes its easier to parse comments if they are in
multiple small emails.  I don't mind getting lots of mail :)

> 
> >
> > if ((flags & PATHSPEC_SYMLINK_LEADING_PATH) &&
> > has_symlink_leading_path(item[i].match, item[i].len)) {
> -- 
> Duy

-- 
Brandon Williams

Re: [PATCH 11/17] pathspec: factor global magic into its own function

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > Create helper functions to read the global magic environment variables
> > in additon to factoring out the global magic gathering logic into its
> > own function.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  pathspec.c | 120 
> > +
> >  1 file changed, 74 insertions(+), 46 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 5afebd3..08e76f6 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -87,6 +87,74 @@ static void prefix_magic(struct strbuf *sb, int 
> > prefixlen, unsigned magic)
> > strbuf_addf(sb, ",prefix:%d)", prefixlen);
> >  }
> >
> > +static inline int get_literal_global(void)
> > +{
> > +   static int literal_global = -1;
> > +
> > +   if (literal_global < 0)
> > +   literal_global = 
> > git_env_bool(GIT_LITERAL_PATHSPECS_ENVIRONMENT,
> > + 0);
> 
> These zeros look so lonely. I know it would exceed 80 columns if we
> put it on the previous line. But I think it's ok for occasional
> exceptions. Or you could rename noglob_global to noglob.

I was thinking the same thing but was so torn between the char limit.  I
think it's probably ok to rename these vars by drooping the global since
the function name themselves indicate they are global.
-- 
Brandon Williams

Re: [PATCH 16/17] pathspec: small readability changes

[Brandon Williams]

On 12/07, Duy Nguyen wrote:
> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
> > A few small changes to improve readability.  This is done by grouping 
> > related
> > assignments, adding blank lines, ensuring lines are <80 characters, etc.
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  pathspec.c | 15 ++-
> >  1 file changed, 10 insertions(+), 5 deletions(-)
> >
> > diff --git a/pathspec.c b/pathspec.c
> > index 41aa213..8a07b02 100644
> > --- a/pathspec.c
> > +++ b/pathspec.c
> > @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
> > *item,
> 
> btw, since this function has stopped being "just prefix pathspec" for
> a long time, perhaps rename it to parse_pathspec_item, or something.

I was thinking about doing that after I sent this out.  Glad you also
pointed that out.

-- 
Brandon Williams

Re: [PATCHv5 0/5] submodule embedgitdirs

[Junio C Hamano]

Stefan Beller  writes:

> v5:
> * Add another layer of abstraction, i.e. the relocate_git_dir is only about 
>   moving a git dir of one repository. The submodule specific stuff (e.g.
>   recursion into nested submodules) is in submodule.{c,h}
>   
>   This was motivated by reviews on the series of checkout aware of submodules
>   building on top of this series, as we want to directly call the 
> embed-git-dirs
>   function without the overhead of spawning a child process.

OK.  Comparing the last steps between this round and the previous
one, I do think the separation of the responsibility among helpers
is much more reasonable in this version, where:

 - submodule_embed_git_dir() is given a single path and is
   responsible for that submodule itself, which is done by calling
   submodule_embed_git_dir_for_path() on itself, and its
   sub-submodules, which is done by spawning the helper recursively
   with appropriate super-prefix;

 - submodule_embed_git_dir_for_path() computes where the given path
   needs to be moved to using the knowledge specific to the
   submodule subsystem, and asks relocate_gitdir() to perform the
   actual relocation;

 - relocate_gitdir() used to do quite a lot more, but now it is only
   about moving an existing .git directory elsewhere and pointing to
   the new location with .git file placed in the old location.

I would have called the second helper submodule_embed_one_git_dir(),
but that is a minor detail.

Very nicely done.

Re: [PATCH] real_path: make real_path thread-safe

[Brandon Williams]

On 12/07, Johannes Sixt wrote:
> Am 07.12.2016 um 01:10 schrieb Brandon Williams:
> >This function should accept both absolute and relative paths, which
> >means it should probably accept "C:\My Files".  I wasn't thinking about
> >windows 100% of the time while writing this so I'm hoping that a windows
> >expert will point things like this out to me :).
> 
> ;)
> 
> With this patch, the test suite fails at the very first git init call:
> 
> D:\Src\mingw-git\t>sh t-basic.sh -v -i -x
> fatal: Invalid path '/:': No such file or directory
> error: cannot run git init -- have you built things yet?
> FATAL: Unexpected exit with code 1
> 
> I haven't dug further, yet.
> 
> -- Hannes
> 

Thanks for providing me with the error.  Instead of assuming root is "/"
I'll need to extract what root is from an absolute path.  Aside from
what root looks like, do most other path constructs behave similarly in
unix and windows? (like ".." and "." as examples)

Since I don't really have a windows machine to test things it might be
slightly difficult to get everything correct quickly but hopefully we can
get this working :)

-- 
Brandon Williams

Re: [PATCH] real_path: make real_path thread-safe

[Brandon Williams]

On 12/07, Junio C Hamano wrote:
> Torsten Bögershausen  writes:
> 
> > But in any case it seems that e.g.
> > //SEFVER/SHARE/DIR1/DIR2/..
> > must be converted into
> > //SEFVER/SHARE/DIR1
> >
> > and 
> > \\SEFVER\SHARE\DIR1\DIR2\..
> > must be converted into
> > \\SEFVER\SHARE\DIR1
> 
> Additional questions that may be interesting are:
> 
> //A/B/../Cis it //A/C?  is it an error?
> //A/B/../../C/D   is it //C/D?  is it an error?
> 


Also is //.. the same as //?  I would assume so since /.. is /

-- 
Brandon Williams

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 1:24 PM, vi0oss  wrote:
> On 12/07/2016 11:09 PM, Stefan Beller wrote:
>>>
>>> As submodule's alternate target does not end in .git/objects
>>> (rather .git/modules/qq/objects), this alternate target
>>> path restriction for in add_possible_reference_from_superproject
>>> relates from "*.git/objects" to just */objects".
>>
>> I wonder if this check is too weak and we actually have to check for
>> either .git/objects or modules//objects.
>> When writing the referenced commit I assumed we'd need a stronger check
>> to be safer and not add some random location as a possible alternate.
>>
> 1. Do we really need to check that it is named ".git"? Although
> "git clone --mirror --recursive" is not (directly) supported
> now, user may create one bare repository with [remnants of]
> submodules by converting reqular repository into bare one.
> Why not take advantage of additional information available locally
> in this case?

Oh, great point.

>
> 2. Is the check need to be strict because of we need to traverse
> one directory level up? Normally this "/objects" part is added by
> Git, so just one "../" seems to be OK. User can't specify "--reference
> somerepo/.git/objects", a strange reference can appear only if user
> manually creates alternates. Maybe better to document this case
> instead of restricting the feature?

Not sure I understand what needs better documentation here?

>
> 3. If nonetheless check for ".git/*/objects", then
> a. What functions should be used in Git codebase for such checks?
> b. Should we handle tricks like "smth/.git/../../objects" and so on?

I see we're getting into problems here.

>
> 4. Should we print (or print in verbose mode) each used alternate,
> to inform operator what his or her new clone will depend on?
>
> P.S. Actually I discovered the --recursive --reference feature when tried to
> put reference to a mega-repo with all possible submodules added as remotes.
> I expected --reference to just get though across all submodules, but it
> complained
> to missing "/modules/..." instead (the check went though becase the
> repository
> was named like "megarepo.git", so it did ended in ".git/objects").

Oh :(

With that said, I think the original patch is a sensible approach.

Thanks,
Stefan

Re: [PATCH 2/2] describe: add support for multiple match patterns

[Junio C Hamano]

Jacob Keller  writes:

> ... Suppose that you version all
> your official releases such as "v1.2", "v1.3", "v1.4", "v2.1" and so on.
> Now, you also have other tags which represent -rc releases and other
> such tags. If you want to find the first major release that contains
> a given commit you might try
>
> git describe --contains --match="v?.?" 
>
> This will work as long as you have only single digits. But if you start
> adding multiple digits, the pattern becomes not enough to match all the
> tags you wanted while excluding the ones you didn't.

Isn't what you really want for the use case a negative pattern,
i.e. "I want ones that match v* but not the ones that match *-rc*",
though?

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Stefan Beller]

On Wed, Dec 7, 2016 at 1:08 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> So my first question I had to answer was if we do the right thing here,
>> i.e. if we could just fail instead. But we want to continue and just
>> not write back the index, which is fine.
>>
>> So we do not have to guard refresh_cache, but just call
>> update_index_if_able conditionally.
>
> An explanation with stepping back a little bit may help.
>
> You may be asked to visit a repository of a friend, to which you do
> not have write access to but you can still read.  You may want to do
> "diff", "status" or "describe" there.
>
> In order to avoid getting fooled into thinking some paths are dirty
> only because the cached stat information does not match, these need
> to refresh the in-core index before doing their "comparison" to
> report which paths are different (in "diff"), what are the modified
> but not staged paths (in "status"), and if there is a need to add
> the "-dirty" suffix (in "describe").
>
> Since we are doing the expensive "bunch of lstat()" anyway, if we
> could write it back to the index, it would help future operations in
> the same repository--that is the reasoning behind the opportunistic
> updates.  It is perfectly OK if we do not have write access to the
> repository and cannot write update the index.
>

Thanks for that explanation!
So I agree that we should not call it _if_needed, but the _if_able
part is still confusing, as we check for different parts here.

The case of not being able to write (read only) sounds similar to
the case of not being able to write (a concurrent lock),
I'll think about it more.

Thanks,
Stefan

Re: [PATCH 16/17] pathspec: small readability changes

[Junio C Hamano]

Duy Nguyen  writes:

> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  wrote:
>> A few small changes to improve readability.  This is done by grouping related
>> assignments, adding blank lines, ensuring lines are <80 characters, etc.
>>
>> Signed-off-by: Brandon Williams 
>> ---
>>  pathspec.c | 15 ++-
>>  1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/pathspec.c b/pathspec.c
>> index 41aa213..8a07b02 100644
>> --- a/pathspec.c
>> +++ b/pathspec.c
>> @@ -334,6 +334,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
>> *item,
>
> btw, since this function has stopped being "just prefix pathspec" for
> a long time, perhaps rename it to parse_pathspec_item, or something.

Not specifically responding to this comment, but thanks for all the
constructive feedback messages.

Re: [PATCH 00/17] pathspec cleanup

[Junio C Hamano]

Brandon Williams  writes:

> The intent of this series is to cleanup some of the pathspec initialization
> code as well as finally migrating the remaining users of the _raw field or
> get_pathspec() to the pathspec struct interface.  This way both the _raw field
> and get_pathspec() can be removed from the codebase.  This also removes the
> functionality where parse_pathspec() modified the const char * argv array that
> was passed in (which felt kind of odd to me as I wouldn't have expected the
> passed in array to be modified).
>
> I also noticed that there are memory leaks associated with the 'original' and
> 'match' strings.  To fix this the pathspec struct needed to take ownership of
> the memory for these fields so that they can be cleaned up when clearing the
> pathspec struct.

Both good goals.  Thanks for working on this.

[PATCH 4/5] Make sequencer abort safer

[Stephan Beyer]

In contrast to "git am --abort", a sequencer abort did not check
whether the current HEAD is the one that is expected. This can
lead to loss of work (when not spotted and resolved using reflog
before the garbage collector chimes in).

This behavior is now changed by mimicking "git am --abort":
the abortion is done but HEAD is not changed when the current HEAD
is not the expected HEAD.

A new file "sequencer/current" is added to save the expected HEAD.

The new behavior is only active when --abort is invoked on multiple
picks. The problem does not occur for the single-pick case because
it is handled differently.

Signed-off-by: Stephan Beyer 
---
 sequencer.c | 49 +
 1 file changed, 49 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index 30b10ba14..c9b560ac1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
 static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
 static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
+static GIT_PATH_FUNC(git_path_curr_file, "sequencer/current")
 
 /*
  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)
return -1;
 }
 
+static void update_curr_file()
+{
+   struct object_id head;
+
+   /* Do nothing on a single-pick */
+   if (!file_exists(git_path_seq_dir()))
+   return;
+
+   if (!get_oid("HEAD", ))
+   write_file(git_path_curr_file(), "%s", oid_to_hex());
+   else
+   write_file(git_path_curr_file(), "%s", "");
+}
+
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
 {
@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const 
unsigned char *from,
strbuf_release();
strbuf_release();
ref_transaction_free(transaction);
+   update_curr_file();
return 0;
 }
 
@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
 
 leave:
free_message(commit, );
+   update_curr_file();
 
return res;
 }
@@ -1132,9 +1149,34 @@ static int save_head(const char *head)
return 0;
 }
 
+static int rollback_is_safe()
+{
+   struct strbuf sb = STRBUF_INIT;
+   struct object_id expected_head, actual_head;
+
+   if (strbuf_read_file(, git_path_curr_file(), 0) >= 0) {
+   strbuf_trim();
+   if (get_oid_hex(sb.buf, _head)) {
+   strbuf_release();
+   die(_("could not parse %s"), git_path_curr_file());
+   }
+   strbuf_release();
+   }
+   else if (errno == ENOENT)
+   oidclr(_head);
+   else
+   die_errno(_("could not read '%s'"), git_path_curr_file());
+
+   if (get_oid("HEAD", _head))
+   oidclr(_head);
+
+   return !oidcmp(_head, _head);
+}
+
 static int reset_for_rollback(const unsigned char *sha1)
 {
const char *argv[4];/* reset --merge  + NULL */
+
argv[0] = "reset";
argv[1] = "--merge";
argv[2] = sha1_to_hex(sha1);
@@ -1189,6 +1231,12 @@ int sequencer_rollback(struct replay_opts *opts)
error(_("cannot abort from a branch yet to be born"));
goto fail;
}
+
+   if (!rollback_is_safe()) {
+   /* Do not error, just do not rollback */
+   warning(_("You seem to have moved HEAD. "
+ "Not rewinding, check your HEAD!"));
+   } else
if (reset_for_rollback(sha1))
goto fail;
strbuf_release();
@@ -1393,6 +1441,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
+   update_curr_file();
res = pick_commits(_list, opts);
todo_list_release(_list);
return res;
-- 
2.11.0.27.g4eed97c

[PATCH 3/5] Add test that cherry-pick --abort does not unsafely change HEAD

[Stephan Beyer]

Signed-off-by: Stephan Beyer 
---
 t/t3510-cherry-pick-sequence.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89dbd..372307c21 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' 
'
git diff-index --exit-code HEAD
 '
 
+test_expect_success '--abort does not unsafely change HEAD' '
+   pristine_detach initial &&
+   test_must_fail git cherry-pick picked anotherpick &&
+   git reset --hard base &&
+   test_must_fail git cherry-pick picked anotherpick &&
+   git cherry-pick --abort 2>actual &&
+   test_i18ngrep "You seem to have moved HEAD" actual &&
+   test_cmp_rev base HEAD
+'
+
 test_expect_success 'cherry-pick --abort to cancel multiple revert' '
pristine_detach anotherpick &&
test_expect_code 1 git revert base..picked &&
-- 
2.11.0.27.g4eed97c

[PATCH 1/5] am: Fix filename in safe_to_abort() error message

[Stephan Beyer]

Signed-off-by: Stephan Beyer 
---
 Okay let's give it a try. Some minor things that I found
 are also in this patchset (patch 01, 02 and 05).
 The branch can also be found on
   https://github.com/sbeyer/git/commits/sequencer-abort-safety

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

diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce..7cf40e6f2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)
 
if (read_state_file(, state, "abort-safety", 1) > 0) {
if (get_oid_hex(sb.buf, _safety))
-   die(_("could not parse %s"), am_path(state, 
"abort_safety"));
+   die(_("could not parse %s"), am_path(state, 
"abort-safety"));
} else
oidclr(_safety);
 
-- 
2.11.0.27.g4eed97c

[PATCH 5/5] sequencer: Remove useless get_dir() function

[Stephan Beyer]

This function is used only once, for the removal of the
directory. It is not used for the creation of the directory
nor anywhere else.

Signed-off-by: Stephan Beyer 
---
 sequencer.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index c9b560ac1..689cfa5f1 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts)
return 0;
 }
 
-static const char *get_dir(const struct replay_opts *opts)
-{
-   return git_path_seq_dir();
-}
-
 static const char *get_todo_path(const struct replay_opts *opts)
 {
return git_path_todo_file();
@@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts)
free(opts->xopts[i]);
free(opts->xopts);
 
-   strbuf_addf(, "%s", get_dir(opts));
+   strbuf_addf(, "%s", git_path_seq_dir());
remove_dir_recursively(, 0);
strbuf_release();
 
-- 
2.11.0.27.g4eed97c

[PATCH 2/5] am: Change safe_to_abort()'s not rewinding error into a warning

[Stephan Beyer]

The error message tells the user that something went terribly wrong
and the --abort could not be performed. But the --abort is performed,
only without rewinding. By simply changing the error into a warning,
we indicate the user that she must not try something like
"git am --abort --force", instead she just has to check the HEAD.

Signed-off-by: Stephan Beyer 
---
 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 7cf40e6f2..826f18ba1 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state)
if (!oidcmp(, _safety))
return 1;
 
-   error(_("You seem to have moved HEAD since the last 'am' failure.\n"
+   warning(_("You seem to have moved HEAD since the last 'am' failure.\n"
"Not rewinding to ORIG_HEAD"));
 
return 0;
-- 
2.11.0.27.g4eed97c

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[vi0oss]

On 12/07/2016 11:09 PM, Stefan Beller wrote:

As submodule's alternate target does not end in .git/objects
(rather .git/modules/qq/objects), this alternate target
path restriction for in add_possible_reference_from_superproject
relates from "*.git/objects" to just */objects".

I wonder if this check is too weak and we actually have to check for
either .git/objects or modules//objects.
When writing the referenced commit I assumed we'd need a stronger check
to be safer and not add some random location as a possible alternate.


1. Do we really need to check that it is named ".git"? Although
"git clone --mirror --recursive" is not (directly) supported
now, user may create one bare repository with [remnants of]
submodules by converting reqular repository into bare one.
Why not take advantage of additional information available locally
in this case?

2. Is the check need to be strict because of we need to traverse
one directory level up? Normally this "/objects" part is added by
Git, so just one "../" seems to be OK. User can't specify "--reference
somerepo/.git/objects", a strange reference can appear only if user
manually creates alternates. Maybe better to document this case
instead of restricting the feature?

3. If nonetheless check for ".git/*/objects", then
a. What functions should be used in Git codebase for such checks?
b. Should we handle tricks like "smth/.git/../../objects" and so on?

4. Should we print (or print in verbose mode) each used alternate,
to inform operator what his or her new clone will depend on?

P.S. Actually I discovered the --recursive --reference feature when tried to
put reference to a mega-repo with all possible submodules added as remotes.
I expected --reference to just get though across all submodules, but it 
complained
to missing "/modules/..." instead (the check went though becase the 
repository

was named like "megarepo.git", so it did ended in ".git/objects").

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Junio C Hamano]

Stefan Beller  writes:

> So my first question I had to answer was if we do the right thing here,
> i.e. if we could just fail instead. But we want to continue and just
> not write back the index, which is fine.
>
> So we do not have to guard refresh_cache, but just call
> update_index_if_able conditionally.

An explanation with stepping back a little bit may help.

You may be asked to visit a repository of a friend, to which you do
not have write access to but you can still read.  You may want to do
"diff", "status" or "describe" there.

In order to avoid getting fooled into thinking some paths are dirty
only because the cached stat information does not match, these need
to refresh the in-core index before doing their "comparison" to
report which paths are different (in "diff"), what are the modified
but not staged paths (in "status"), and if there is a need to add
the "-dirty" suffix (in "describe").

Since we are doing the expensive "bunch of lstat()" anyway, if we
could write it back to the index, it would help future operations in
the same repository--that is the reasoning behind the opportunistic
updates.  It is perfectly OK if we do not have write access to the
repository and cannot write update the index.

[PATCHv5 1/5] submodule: use absolute path for computing relative path connecting

[Stefan Beller]

The current caller of connect_work_tree_and_git_dir passes
an absolute path for the `git_dir` parameter. In the future patch
we will also pass in relative path for `git_dir`. Extend the functionality
of connect_work_tree_and_git_dir to take relative paths for parameters.

We could work around this in the future patch by computing the absolute
path for the git_dir in the calling site, however accepting relative
paths for either parameter makes the API for this function much harder
to misuse.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 submodule.c | 12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/submodule.c b/submodule.c
index 6f7d883de9..66c5ce5a24 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char 
*work_tree, const char *git_dir)
 {
struct strbuf file_name = STRBUF_INIT;
struct strbuf rel_path = STRBUF_INIT;
-   const char *real_work_tree = xstrdup(real_path(work_tree));
+   char *real_git_dir = xstrdup(real_path(git_dir));
+   char *real_work_tree = xstrdup(real_path(work_tree));
 
/* Update gitfile */
strbuf_addf(_name, "%s/.git", work_tree);
write_file(file_name.buf, "gitdir: %s",
-  relative_path(git_dir, real_work_tree, _path));
+  relative_path(real_git_dir, real_work_tree, _path));
 
/* Update core.worktree setting */
strbuf_reset(_name);
-   strbuf_addf(_name, "%s/config", git_dir);
+   strbuf_addf(_name, "%s/config", real_git_dir);
git_config_set_in_file(file_name.buf, "core.worktree",
-  relative_path(real_work_tree, git_dir,
+  relative_path(real_work_tree, real_git_dir,
 _path));
 
strbuf_release(_name);
strbuf_release(_path);
-   free((void *)real_work_tree);
+   free(real_work_tree);
+   free(real_git_dir);
 }
 
 int parallel_submodules(void)
-- 
2.11.0.rc2.28.g2af45f1.dirty

[PATCHv5 0/5] submodule embedgitdirs

[Stefan Beller]

v5:
* Add another layer of abstraction, i.e. the relocate_git_dir is only about 
  moving a git dir of one repository. The submodule specific stuff (e.g.
  recursion into nested submodules) is in submodule.{c,h}
  
  This was motivated by reviews on the series of checkout aware of submodules
  building on top of this series, as we want to directly call the embed-git-dirs
  function without the overhead of spawning a child process.
  
Thanks,
Stefan
 
v4:
* rebuilt on top of nd/worktree-list-fixup
* fix and test behavior for un-init submodules (don't crash, rather do nothing)
* incorporated a "static" as pointed out by Ramsay
* use internal functions instead of duplicating code in worktree.c
  (use get_common_dir_noenv for the submodule to actually get the common dir)
* fixed a memory leak in relocate_gitdir

v3:
* have a slightly more generic function "relocate_gitdir".
  The recursion is strictly related to submodules, though.
* bail out if a submodule is using worktrees.
  This also lays the groundwork for later doing the proper thing,
  as worktree.h offers a function `get_submodule_worktrees(path)`
* nit by duy: use git_path instead of git_common_dir

v2:
* fixed commit message for patch:
 "submodule: use absolute path for computing relative path connecting"
* a new patch "submodule helper: support super prefix"
* redid the final patch with more tests and fixing bugs along the way
* "test-lib-functions.sh: teach test_commit -C " unchanged

v1:
The discussion of the submodule checkout series revealed to me that a command
is needed to move the git directory from the submodules working tree to be
embedded into the superprojects git directory.

So I wrote the code to intern the submodules git dir into the superproject,
but whilst writing the code I realized this could be valueable for our use
in testing too. So I exposed it via the submodule--helper. But as the
submodule helper ought to be just an internal API, we could also
offer it via the proper submodule command.

The command as it is has little value to the end user for now, but
breaking it out of the submodule checkout series hopefully makes review easier.

Thanks,
Stefan

Stefan Beller (5):
  submodule: use absolute path for computing relative path connecting
  submodule helper: support super prefix
  test-lib-functions.sh: teach test_commit -C 
  worktree: get worktrees from submodules
  submodule: add embed-git-dir function

 Documentation/git-submodule.txt   |  14 +
 builtin/submodule--helper.c   |  68 
 dir.c |  27 
 dir.h |   3 +
 git-submodule.sh  |   7 ++-
 git.c |   2 +-
 submodule.c   | 127 --
 submodule.h   |   7 +++
 t/t7412-submodule-embedgitdirs.sh | 101 ++
 t/test-lib-functions.sh   |  20 --
 worktree.c|  47 +++---
 worktree.h|   6 ++
 12 files changed, 396 insertions(+), 33 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh
 
diff to v4:

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 10df69c86a..321c9e250a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1106,31 +1106,8 @@ static int embed_git_dir(int argc, const char **argv, 
const char *prefix)
if (module_list_compute(argc, argv, prefix, , ) < 0)
return 1;
 
-   for (i = 0; i < list.nr; i++) {
-   const char *path = list.entries[i]->name, *sub_git_dir, *v;
-   char *real_sub_git_dir = NULL, *real_common_git_dir = NULL;
-   struct strbuf gitdir = STRBUF_INIT;
-
-   strbuf_addf(, "%s/.git", path);
-   sub_git_dir = resolve_gitdir(gitdir.buf);
-
-   /* not populated? */
-   if (!sub_git_dir)
-   goto free_and_continue;
-
-   /* Is it already embedded? */
-   real_sub_git_dir = xstrdup(real_path(sub_git_dir));
-   real_common_git_dir = xstrdup(real_path(get_git_common_dir()));
-   if (skip_prefix(real_sub_git_dir, real_common_git_dir, ), 
NULL)
-   goto free_and_continue;
-
-   relocate_gitdir(prefix, path, flags);
-
-free_and_continue:
-   strbuf_release();
-   free(real_sub_git_dir);
-   free(real_common_git_dir);
-   }
+   for (i = 0; i < list.nr; i++)
+   submodule_embed_git_dir(prefix, list.entries[i]->name, flags);
 
return 0;
 }
diff --git a/dir.c b/dir.c
index d2f60b5abf..e023b04407 100644
--- a/dir.c
+++ b/dir.c
@@ -15,9 +15,6 @@
 #include "utf8.h"
 #include "varint.h"
 #include "ewah/ewok.h"
-#include "submodule-config.h"
-#include "run-command.h"
-#include "worktree.h"
 
 struct path_simplify {
int len;
@@ -2753,79 

[PATCHv5 3/5] test-lib-functions.sh: teach test_commit -C

[Stefan Beller]

Specifically when setting up submodule tests, it comes in handy if
we can create commits in repositories that are not at the root of
the tested trash dir. Add "-C " similar to gits -C parameter
that will perform the operation in the given directory.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 t/test-lib-functions.sh | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fdaeb3a96b..579e812506 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -157,16 +157,21 @@ debug () {
 GIT_TEST_GDB=1 "$@"
 }
 
-# Call test_commit with the arguments " [ [ []]]"
+# Call test_commit with the arguments
+# [-C ]  [ [ []]]"
 #
 # This will commit a file with the given contents and the given commit
 # message, and tag the resulting commit with the given tag name.
 #
 # , , and  all default to .
+#
+# If the first argument is "-C", the second argument is used as a path for
+# the git invocations.
 
 test_commit () {
notick= &&
signoff= &&
+   indir= &&
while test $# != 0
do
case "$1" in
@@ -176,21 +181,26 @@ test_commit () {
--signoff)
signoff="$1"
;;
+   -C)
+   indir="$2"
+   shift
+   ;;
*)
break
;;
esac
shift
done &&
+   indir=${indir:+"$indir"/} &&
file=${2:-"$1.t"} &&
-   echo "${3-$1}" > "$file" &&
-   git add "$file" &&
+   echo "${3-$1}" > "$indir$file" &&
+   git ${indir:+ -C "$indir"} add "$file" &&
if test -z "$notick"
then
test_tick
fi &&
-   git commit $signoff -m "$1" &&
-   git tag "${4:-$1}"
+   git ${indir:+ -C "$indir"} commit $signoff -m "$1" &&
+   git ${indir:+ -C "$indir"} tag "${4:-$1}"
 }
 
 # Call test_merge with the arguments " ", where 
-- 
2.11.0.rc2.28.g2af45f1.dirty

[PATCHv5 2/5] submodule helper: support super prefix

[Stefan Beller]

Just like main commands in Git, the submodule helper needs
access to the superproject prefix. Enable this in the git.c
but have its own fuse in the helper code by having a flag to
turn on the super prefix.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/submodule--helper.c | 31 ---
 git.c   |  2 +-
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5f9f..806e29ce4e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,21 +1076,24 @@ static int resolve_remote_submodule_branch(int argc, 
const char **argv,
return 0;
 }
 
+#define SUPPORT_SUPER_PREFIX (1<<0)
+
 struct cmd_struct {
const char *cmd;
int (*fn)(int, const char **, const char *);
+   int option;
 };
 
 static struct cmd_struct commands[] = {
-   {"list", module_list},
-   {"name", module_name},
-   {"clone", module_clone},
-   {"update-clone", update_clone},
-   {"relative-path", resolve_relative_path},
-   {"resolve-relative-url", resolve_relative_url},
-   {"resolve-relative-url-test", resolve_relative_url_test},
-   {"init", module_init},
-   {"remote-branch", resolve_remote_submodule_branch}
+   {"list", module_list, 0},
+   {"name", module_name, 0},
+   {"clone", module_clone, 0},
+   {"update-clone", update_clone, 0},
+   {"relative-path", resolve_relative_path, 0},
+   {"resolve-relative-url", resolve_relative_url, 0},
+   {"resolve-relative-url-test", resolve_relative_url_test, 0},
+   {"init", module_init, 0},
+   {"remote-branch", resolve_remote_submodule_branch, 0}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
@@ -1100,9 +1103,15 @@ int cmd_submodule__helper(int argc, const char **argv, 
const char *prefix)
die(_("submodule--helper subcommand must be "
  "called with a subcommand"));
 
-   for (i = 0; i < ARRAY_SIZE(commands); i++)
-   if (!strcmp(argv[1], commands[i].cmd))
+   for (i = 0; i < ARRAY_SIZE(commands); i++) {
+   if (!strcmp(argv[1], commands[i].cmd)) {
+   if (get_super_prefix() &&
+   !(commands[i].option & SUPPORT_SUPER_PREFIX))
+   die("%s doesn't support --super-prefix",
+   commands[i].cmd);
return commands[i].fn(argc - 1, argv + 1, prefix);
+   }
+   }
 
die(_("'%s' is not a valid submodule--helper "
  "subcommand"), argv[1]);
diff --git a/git.c b/git.c
index efa1059fe0..98dcf6c518 100644
--- a/git.c
+++ b/git.c
@@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {
{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
{ "stripspace", cmd_stripspace },
-   { "submodule--helper", cmd_submodule__helper, RUN_SETUP },
+   { "submodule--helper", cmd_submodule__helper, RUN_SETUP | 
SUPPORT_SUPER_PREFIX},
{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
{ "tag", cmd_tag, RUN_SETUP },
{ "unpack-file", cmd_unpack_file, RUN_SETUP },
-- 
2.11.0.rc2.28.g2af45f1.dirty

[PATCHv5 5/5] submodule: add embed-git-dir function

[Stefan Beller]

When a submodule has its git dir inside the working dir, the submodule
support for checkout that we plan to add in a later patch will fail.

Add functionality to migrate the git directory to be embedded
into the superprojects git directory.

The newly added code in this patch is structured such that
other areas of Git can also make use of it. The code in the
submodule--helper is a mere wrapper and option parser
for the function `submodule_embed_git_dir_for_path`, that
takes care of embedding the submodules git directory into
the superprojects git dir. That function makes use of the
more abstract function for this use case `relocate_gitdir`,
which can be used by e.g. the worktree code eventually to
move around a git directory.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/git-submodule.txt   |  14 +
 builtin/submodule--helper.c   |  39 -
 dir.c |  27 +
 dir.h |   3 +
 git-submodule.sh  |   7 ++-
 submodule.c   | 115 ++
 submodule.h   |   7 +++
 t/t7412-submodule-embedgitdirs.sh | 101 +
 8 files changed, 311 insertions(+), 2 deletions(-)
 create mode 100755 t/t7412-submodule-embedgitdirs.sh

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index d841573475..34791cfc65 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -22,6 +22,7 @@ SYNOPSIS
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
 'git submodule' [--quiet] sync [--recursive] [--] [...]
+'git submodule' [--quiet] embedgitdirs [--] [...]
 
 
 DESCRIPTION
@@ -245,6 +246,19 @@ sync::
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and sync any nested submodules within.
 
+embedgitdirs::
+   Move the git directory of submodules into its superprojects
+   `$GIT_DIR/modules` path and then connect the git directory and
+   its working directory by setting the `core.worktree` and adding
+   a .git file pointing to the git directory interned into the
+   superproject.
++
+A repository that was cloned independently and later added as a submodule or
+old setups have the submodules git directory inside the submodule instead of
+embedded into the superprojects git directory.
++
+This command is recursive by default.
+
 OPTIONS
 ---
 -q::
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 806e29ce4e..321c9e250a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1076,6 +1076,42 @@ static int resolve_remote_submodule_branch(int argc, 
const char **argv,
return 0;
 }
 
+static int embed_git_dir(int argc, const char **argv, const char *prefix)
+{
+   int i;
+   struct pathspec pathspec;
+   struct module_list list = MODULE_LIST_INIT;
+   unsigned flags = RELOCATE_GITDIR_RECURSE_SUBMODULES;
+
+   struct option embed_gitdir_options[] = {
+   OPT_STRING(0, "prefix", ,
+  N_("path"),
+  N_("path into the working tree")),
+   OPT_BIT(0, "--recursive", , N_("recurse into submodules"),
+   RELOCATE_GITDIR_RECURSE_SUBMODULES),
+   OPT_END()
+   };
+
+   const char *const git_submodule_helper_usage[] = {
+   N_("git submodule--helper embed-git-dir [...]"),
+   NULL
+   };
+
+   argc = parse_options(argc, argv, prefix, embed_gitdir_options,
+git_submodule_helper_usage, 0);
+
+   gitmodules_config();
+   git_config(submodule_config, NULL);
+
+   if (module_list_compute(argc, argv, prefix, , ) < 0)
+   return 1;
+
+   for (i = 0; i < list.nr; i++)
+   submodule_embed_git_dir(prefix, list.entries[i]->name, flags);
+
+   return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
{"init", module_init, 0},
-   {"remote-branch", resolve_remote_submodule_branch, 0}
+   {"remote-branch", resolve_remote_submodule_branch, 0},
+   {"embed-git-dirs", embed_git_dir, SUPPORT_SUPER_PREFIX}
 };
 
 int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
diff --git a/dir.c b/dir.c
index bfa8c8a9a5..e023b04407 100644
--- a/dir.c
+++ b/dir.c
@@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state 
*istate,
 {
untracked_cache_invalidate_path(istate, path);
 }
+
+/*
+ * Migrate the git directory of the given `path` from `old_git_dir` to
+ * `new_git_dir`. If an error occurs, append it to `err` and 

[PATCHv5 4/5] worktree: get worktrees from submodules

[Stefan Beller]

In a later patch we want to move around the the git directory of
a submodule. Both submodules as well as worktrees are involved in
placing git directories at unusual places, so their functionality
may collide. To react appropriately to situations where worktrees
in submodules are in use, offer a new function to query the
worktrees for submodules.

Signed-off-by: Stefan Beller 
---
 worktree.c | 47 +--
 worktree.h |  6 ++
 2 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/worktree.c b/worktree.c
index eb6121263b..fa2b6dfa9a 100644
--- a/worktree.c
+++ b/worktree.c
@@ -72,7 +72,7 @@ static void add_head_info(struct strbuf *head_ref, struct 
worktree *worktree)
 /**
  * get the main worktree
  */
-static struct worktree *get_main_worktree(void)
+static struct worktree *get_main_worktree(const char *git_common_dir)
 {
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -81,12 +81,12 @@ static struct worktree *get_main_worktree(void)
int is_bare = 0;
int is_detached = 0;
 
-   strbuf_add_absolute_path(_path, get_git_common_dir());
+   strbuf_add_absolute_path(_path, git_common_dir);
is_bare = !strbuf_strip_suffix(_path, "/.git");
if (is_bare)
strbuf_strip_suffix(_path, "/.");
 
-   strbuf_addf(, "%s/HEAD", get_git_common_dir());
+   strbuf_addf(, "%s/HEAD", git_common_dir);
 
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(_path, NULL);
@@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
return worktree;
 }
 
-static struct worktree *get_linked_worktree(const char *id)
+static struct worktree *get_linked_worktree(const char *git_common_dir,
+   const char *id)
 {
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
@@ -112,7 +113,7 @@ static struct worktree *get_linked_worktree(const char *id)
if (!id)
die("Missing linked worktree name");
 
-   strbuf_git_common_path(, "worktrees/%s/gitdir", id);
+   strbuf_addf(, "%s/worktrees/%s/gitdir", git_common_dir, id);
if (strbuf_read_file(_path, path.buf, 0) <= 0)
/* invalid gitdir file */
goto done;
@@ -125,7 +126,7 @@ static struct worktree *get_linked_worktree(const char *id)
}
 
strbuf_reset();
-   strbuf_addf(, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
+   strbuf_addf(, "%s/worktrees/%s/HEAD", git_common_dir, id);
 
if (parse_ref(path.buf, _ref, _detached) < 0)
goto done;
@@ -167,7 +168,8 @@ static int compare_worktree(const void *a_, const void *b_)
return fspathcmp((*a)->path, (*b)->path);
 }
 
-struct worktree **get_worktrees(unsigned flags)
+static struct worktree **get_worktrees_internal(const char *git_common_dir,
+   unsigned flags)
 {
struct worktree **list = NULL;
struct strbuf path = STRBUF_INIT;
@@ -177,9 +179,9 @@ struct worktree **get_worktrees(unsigned flags)
 
list = xmalloc(alloc * sizeof(struct worktree *));
 
-   list[counter++] = get_main_worktree();
+   list[counter++] = get_main_worktree(git_common_dir);
 
-   strbuf_addf(, "%s/worktrees", get_git_common_dir());
+   strbuf_addf(, "%s/worktrees", git_common_dir);
dir = opendir(path.buf);
strbuf_release();
if (dir) {
@@ -188,7 +190,7 @@ struct worktree **get_worktrees(unsigned flags)
if (is_dot_or_dotdot(d->d_name))
continue;
 
-   if ((linked = get_linked_worktree(d->d_name))) {
+   if ((linked = get_linked_worktree(git_common_dir, 
d->d_name))) {
ALLOC_GROW(list, counter + 1, alloc);
list[counter++] = linked;
}
@@ -209,6 +211,31 @@ struct worktree **get_worktrees(unsigned flags)
return list;
 }
 
+struct worktree **get_worktrees(unsigned flags)
+{
+   return get_worktrees_internal(get_git_common_dir(), flags);
+}
+
+struct worktree **get_submodule_worktrees(const char *path, unsigned flags)
+{
+   char *submodule_gitdir;
+   const char *submodule_common_dir;
+   struct strbuf sb = STRBUF_INIT;
+   struct worktree **ret;
+
+   submodule_gitdir = git_pathdup_submodule(path, "%s", "");
+   if (!submodule_gitdir)
+   return NULL;
+
+   /* the env would be set for the superproject */
+   get_common_dir_noenv(, submodule_gitdir);
+   submodule_common_dir = strbuf_detach(, NULL);
+   ret = get_worktrees_internal(submodule_common_dir, flags);
+
+   free(submodule_gitdir);
+   return ret;
+}
+
 const char *get_worktree_git_dir(const struct worktree *wt)
 {
if (!wt)
diff --git a/worktree.h 

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Stefan Beller]

On Wed, Dec 7, 2016 at 11:41 AM, Junio C Hamano  wrote:
> The require_clean_work_tree() function calls hold_locked_index()
> with die_on_error=0 to signal that it is OK if it fails to obtain
> the lock, but unconditionally calls update_index_if_able(), which
> will try to write into fd=-1.
>
> Signed-off-by: Junio C Hamano 
> ---

So my first question I had to answer was if we do the right thing here,
i.e. if we could just fail instead. But we want to continue and just
not write back the index, which is fine.

So we do not have to guard refresh_cache, but just call
update_index_if_able conditionally.

However I think the promise of that function is
to take care of the fd == -1?

/*
* Opportunistically update the index but do not complain if we can't
*/
void update_index_if_able(struct index_state *istate, struct
lock_file *lockfile)
{
if ((istate->cache_changed || has_racy_timestamp(istate)) &&
verify_index(istate) &&
write_locked_index(istate, lockfile, COMMIT_LOCK))
rollback_lock_file(lockfile);
}

So I would expect that we'd rather fix the update_index_if_able instead by
checking for the lockfile to be in the correct state?


>  wt-status.c | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index a2e9d332d8..a715e71906 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -2258,11 +2258,12 @@ int has_uncommitted_changes(int ignore_submodules)
>  int require_clean_work_tree(const char *action, const char *hint, int 
> ignore_submodules, int gently)
>  {
> struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> -   int err = 0;
> +   int err = 0, fd;
>
> -   hold_locked_index(lock_file, 0);
> +   fd = hold_locked_index(lock_file, 0);
> refresh_cache(REFRESH_QUIET);
> -   update_index_if_able(_index, lock_file);
> +   if (0 <= fd)
> +   update_index_if_able(_index, lock_file);
> rollback_lock_file(lock_file);
>
> if (has_unstaged_changes(ignore_submodules)) {
> --
> 2.11.0-274-g0631465056
>

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Stefan Beller]

On Wed, Dec 7, 2016 at 12:53 PM, Junio C Hamano  wrote:
> On Wed, Dec 7, 2016 at 12:48 PM, Stefan Beller  wrote:
>>
>> So I would expect that we'd rather fix the update_index_if_able instead by
>> checking for the lockfile to be in the correct state?
>
> I actually don't expect that, after looking at other call sites of
> that function.

Yes I checked the other callers as well right now and you seem to be correct.
My initial response was based on the name of the function,
specifically the _if_able
part as that hinted to me that I can call the function with no
precondition and the
_if_able will figure out when to do the actual update_index.

The first part of the condition of the function
(istate->cache_changed || has_racy_timestamp(istate)
reads rather as a _if_needed instead of an _if_able to me.

Re: [PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Junio C Hamano]

On Wed, Dec 7, 2016 at 12:48 PM, Stefan Beller  wrote:
>
> So I would expect that we'd rather fix the update_index_if_able instead by
> checking for the lockfile to be in the correct state?

I actually don't expect that, after looking at other call sites of
that function.

Re: [PATCH] real_path: make real_path thread-safe

[Johannes Sixt]

Am 07.12.2016 um 01:10 schrieb Brandon Williams:

This function should accept both absolute and relative paths, which
means it should probably accept "C:\My Files".  I wasn't thinking about
windows 100% of the time while writing this so I'm hoping that a windows
expert will point things like this out to me :).


;)

With this patch, the test suite fails at the very first git init call:

D:\Src\mingw-git\t>sh t-basic.sh -v -i -x
fatal: Invalid path '/:': No such file or directory
error: cannot run git init -- have you built things yet?
FATAL: Unexpected exit with code 1

I haven't dug further, yet.

-- Hannes

Re: [PATCH] real_path: make real_path thread-safe

[Junio C Hamano]

Torsten Bögershausen  writes:

> But in any case it seems that e.g.
> //SEFVER/SHARE/DIR1/DIR2/..
> must be converted into
> //SEFVER/SHARE/DIR1
>
> and 
> \\SEFVER\SHARE\DIR1\DIR2\..
> must be converted into
> \\SEFVER\SHARE\DIR1

Additional questions that may be interesting are:

//A/B/../C  is it //A/C?  is it an error?
//A/B/../../C/D is it //C/D?  is it an error?

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Stephan Beyer]

Hi,

On 12/07/2016 09:04 PM, Junio C Hamano wrote:
> Stephan Beyer  writes:
> 
>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>> different wording for the same thing makes things unintuitive.
> 
> It is not too late to STOP "--forget" from getting added to "rebase"
> and give it a better name.

Oh. ;) I am not sure. I personally think that --forget is a better name
than --quit because when I hear --quit I tend to look into the manual
page first to check if there are weird side effects (and then the manual
page says that it "forgets" ;D).
So I'd rather favor adding --forget to cherry-pick/revert instead... or
this:

> Having said that, I have a feeling that these options do not have to
> exist; isn't their presence just a symptom that the "--abort" for
> the command misbehaves?  Isn't the reason why there is no need for
> "am --quit" because its "--abort" behaves more sensibly?
You're probably right. I have no other use-case in mind than "oh I
forgot that I was rebasing... now just abort that and don't bother me
further (i.e. please don't bring me back)"

~Stephan

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Junio C Hamano]

Stefan Beller  writes:

> On Wed, Dec 7, 2016 at 12:18 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
 This patch makes all not just the root repository, but also
 all submodules (recursively) have submodule.alternateLocation
 and submodule.alternateErrorStrategy configured, making Git
 search for possible alternates for nested submodules as well.
>>>
>>> Sounds great!
>>
>> Is it safe to assume that all the submodules used recursively by
>> submodules share the same structure upstream?  Does the alternate
>> location mechanism degrades sensibly if this assumption turns out to
>> be false (i.e. "possible alternates" above turns out to be mere
>> possibility and not there)?
>
> According to the last test in the patch, this seems to be doing the
> sensible thing.

OK, that sounds great.  Thanks.

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 12:18 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> This patch makes all not just the root repository, but also
>>> all submodules (recursively) have submodule.alternateLocation
>>> and submodule.alternateErrorStrategy configured, making Git
>>> search for possible alternates for nested submodules as well.
>>
>> Sounds great!
>
> Is it safe to assume that all the submodules used recursively by
> submodules share the same structure upstream?  Does the alternate
> location mechanism degrades sensibly if this assumption turns out to
> be false (i.e. "possible alternates" above turns out to be mere
> possibility and not there)?

According to the last test in the patch, this seems to be doing the
sensible thing.

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Junio C Hamano]

Stefan Beller  writes:

>> This patch makes all not just the root repository, but also
>> all submodules (recursively) have submodule.alternateLocation
>> and submodule.alternateErrorStrategy configured, making Git
>> search for possible alternates for nested submodules as well.
>
> Sounds great!

Is it safe to assume that all the submodules used recursively by
submodules share the same structure upstream?  Does the alternate
location mechanism degrades sensibly if this assumption turns out to
be false (i.e. "possible alternates" above turns out to be mere
possibility and not there)?

Re: [PATCH] real_path: make real_path thread-safe

[Torsten Bögershausen]

On Wed, Dec 07, 2016 at 01:12:25AM +, Ramsay Jones wrote:
> 
> 
> On 07/12/16 00:10, Brandon Williams wrote:
> > On 12/06, Junio C Hamano wrote:
> >> POSIX cares about treating "//" at the very beginning of the path
> >> specially.  Is that supposed to be handled here, or by a lot higher
> >> level up in the callchain?
> > 
> > What exactly does "//" mean in this context? (I'm just naive in this
> > area)
> 
> This refers to a UNC path (ie Universal Naming Convention) which
> is used to refer to servers, printers and other 'network resources'.
> Although this started (many moons ago) in unix, it isn't used too
> much outside of windows networks! (where it is usually denoted by
> \\servername\path).
> 
> You can see the relics of unix UNC paths if you look at the wording
> for basename() in the POSIX standard. Note the special treatment of
> the path which 'is exactly "//"', see 
> http://pubs.opengroup.org/onlinepubs/009695399/functions/basename.html
> 
> ATB,
> Ramsay Jones

Please allow one more comment about UNC:
They are used under Windows, and typically wotk together with Git.
One breakage between 2.10 and 2.11 has been observed, saying that
pushing to \\SERVER\SHARE\DIRECTORY does not work any more.

It has been reported under
 "git 2.11.0 error when pushing to remote located on a windows share"
both here and 
here:
https://github.com/git-for-windows/git/issues/979#issuecomment-264816175

I don't have a Windows box at the moment, and I don't know if the
breakage was introduced by changes in real_patyh().

But in any case it seems that e.g.
//SEFVER/SHARE/DIR1/DIR2/..
must be converted into
//SEFVER/SHARE/DIR1

and 
\\SEFVER\SHARE\DIR1\DIR2\..
must be converted into
\\SEFVER\SHARE\DIR1

Re: [PATCH] submodule--helper: set alternateLocation for cloned submodules

[Stefan Beller]

On Wed, Dec 7, 2016 at 10:42 AM,   wrote:
> From: Vitaly _Vi Shukela 

Thanks for contributing to Git!
(/me looks up if you have sent patches already as you
seem to know how to do that. :) unrelated side note: Maybe you want
to send a patch for the .mailmap file mapping your two email addresses
together, c.f. "git log -- .mailmap")

>
> Git v2.11 introduced "git clone --recursive --referece ...",
> but it didn't put the alternates for _nested_ submodules.

This message is targeted at people familiar with gits code base,
so we can be more specific. e.g.

In 31224cbdc7 (clone: recursive and reference option triggers
submodule alternates, 2016-08-17) a mechanism was added to
have submodules referenced.  It did not address _nested_
submodules, however.

>
> This patch makes all not just the root repository, but also
> all submodules (recursively) have submodule.alternateLocation
> and submodule.alternateErrorStrategy configured, making Git
> search for possible alternates for nested submodules as well.

Sounds great!

>
> As submodule's alternate target does not end in .git/objects
> (rather .git/modules/qq/objects), this alternate target
> path restriction for in add_possible_reference_from_superproject
> relates from "*.git/objects" to just */objects".

I wonder if this check is too weak and we actually have to check for
either .git/objects or modules//objects.
When writing the referenced commit I assumed we'd need a stronger check
to be safer and not add some random location as a possible alternate.

>
> New tests have been added to t7408-submodule-reference.

Thanks!

>
> Signed-off-by: Vitaly _Vi Shukela 
> ---
>  builtin/submodule--helper.c| 24 --
>  t/t7408-submodule-reference.sh | 73 
> ++
>  2 files changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 4beeda5..93dae62 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(
>
> /*
>  * If the alternate object store is another repository, try the
> -* standard layout with .git/modules//objects
> +* standard layout with .git/(modules/)+/objects
>  */
> -   if (ends_with(alt->path, ".git/objects")) {
> +   if (ends_with(alt->path, "/objects")) {
> char *sm_alternate;
> struct strbuf sb = STRBUF_INIT;
> struct strbuf err = STRBUF_INIT;
> @@ -672,6 +672,26 @@ static int module_clone(int argc, const char **argv, 
> const char *prefix)
> die(_("could not get submodule directory for '%s'"), path);
> git_config_set_in_file(p, "core.worktree",
>relative_path(path, sm_gitdir, _path));
> +
> +   /* setup alternateLocation and alternateErrorStrategy in the cloned 
> submodule if needed */
> +   {

Usually we do not use braces to further nest code, please remove this nesting.

> +   char *sm_alternate = NULL, *error_strategy = NULL;
> +
> +   git_config_get_string("submodule.alternateLocation", 
> _alternate);
> +   if (sm_alternate) {
> +   git_config_set_in_file(p, 
> "submodule.alternateLocation",
> +  sm_alternate);
> +   }
> +   git_config_get_string("submodule.alternateErrorStrategy", 
> _strategy);
> +   if (error_strategy) {
> +   git_config_set_in_file(p, 
> "submodule.alternateErrorStrategy",
> +  error_strategy);
> +   }
> +
> +   free(sm_alternate);
> +   free(error_strategy);
> +   }
> +
> strbuf_release();
> strbuf_release(_path);
> free(sm_gitdir);
> diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
> index 1c1e289..7b64725 100755
> --- a/t/t7408-submodule-reference.sh
> +++ b/t/t7408-submodule-reference.sh
> @@ -125,4 +125,77 @@ test_expect_success 'ignoring missing submodule 
> alternates passes clone and subm
> )
>  '
>
> +test_expect_success 'preparing second superproject with a nested submodule' '
> +   test_create_repo supersuper &&
> +   (
> +   cd supersuper &&
> +   echo I am super super. >file &&

Usually we quote strings containing white space, e.g. echo "I am ..." >actual

> +   git add file &&
> +   git commit -m B-super-super-initial
> +   git submodule add "file://$base_dir/super" subwithsub &&
> +   git commit -m B-super-super-added &&
> +   git submodule update --init --recursive &&
> +   git repack -ad
> +   ) &&
> +   echo not cleaning supersuper

This echo is left in 

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Junio C Hamano]

Stephan Beyer  writes:

> [1] By the way: git cherry-pick --quit, git rebase --forget ...
> different wording for the same thing makes things unintuitive.

It is not too late to STOP "--forget" from getting added to "rebase"
and give it a better name.

Having said that, I have a feeling that these options do not have to
exist; isn't their presence just a symptom that the "--abort" for
the command misbehaves?  Isn't the reason why there is no need for
"am --quit" because its "--abort" behaves more sensibly?

[PATCH 3/3] lockfile: LOCK_REPORT_ON_ERROR

[Junio C Hamano]

The "libify sequencer" topic stopped passing the die_on_error option
to hold_locked_index(), and this lost an error message from "git
merge --ff-only $commit" when there are competing updates in
progress.

The command still exits with a non-zero status, but that is not of
much help for an interactive user.  The last thing the command says
is "Updating $from..$to".  We used to follow it with a big error
message that makes it clear that "merge --ff-only" did not succeed.

What is sad is that we should have noticed this regression while
reviewing the change.  It was clear that the update to the
checkout_fast_forward() function made a failing hold_locked_index()
silent, but the only caller of the checkout_fast_forward() function
had this comment:

if (checkout_fast_forward(from, to, 1))
-   exit(128); /* the callee should have complained already */
+   return -1; /* the callee should have complained already */

which clearly contradicted the assumption X-<.

Add a new option LOCK_REPORT_ON_ERROR that can be passed instead of
LOCK_DIE_ON_ERROR to the hold_lock*() family of functions and teach
checkout_fast_forward() to use it to fix this regression.

After going thourgh all calls to hold_lock*() family of functions
that used to pass LOCK_DIE_ON_ERROR but were modified to pass 0 in
the "libify sequencer" topic "git show --first-parent 2a4062a4a8",
it appears that this is the only one that has become silent.  Many
others used to give detailed report that talked about "there may be
competing Git process running" but with the series merged they now
only give a single liner "Unable to lock ...", some of which may
have to be tweaked further, but at least they say something, unlike
the one this patch fixes.

Reported-by: Robbie Iannucci 
Signed-off-by: Junio C Hamano 
---
 lockfile.c | 12 ++--
 lockfile.h |  8 +++-
 merge.c|  2 +-
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 9268cdf325..aa69210d8b 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -174,8 +174,16 @@ int hold_lock_file_for_update_timeout(struct lock_file 
*lk, const char *path,
  int flags, long timeout_ms)
 {
int fd = lock_file_timeout(lk, path, flags, timeout_ms);
-   if (fd < 0 && (flags & LOCK_DIE_ON_ERROR))
-   unable_to_lock_die(path, errno);
+   if (fd < 0) {
+   if (flags & LOCK_DIE_ON_ERROR)
+   unable_to_lock_die(path, errno);
+   if (flags & LOCK_REPORT_ON_ERROR) {
+   struct strbuf buf = STRBUF_INIT;
+   unable_to_lock_message(path, errno, );
+   error("%s", buf.buf);
+   strbuf_release();
+   }
+   }
return fd;
 }
 
diff --git a/lockfile.h b/lockfile.h
index d26ad27b2b..16775a7d79 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -129,11 +129,17 @@ struct lock_file {
 /*
  * If a lock is already taken for the file, `die()` with an error
  * message. If this flag is not specified, trying to lock a file that
- * is already locked returns -1 to the caller.
+ * is already locked silently returns -1 to the caller, or ...
  */
 #define LOCK_DIE_ON_ERROR 1
 
 /*
+ * ... this flag can be passed instead to return -1 and give the usual
+ * error message upon an error.
+ */
+#define LOCK_REPORT_ON_ERROR 2
+
+/*
  * Usually symbolic links in the destination path are resolved. This
  * means that (1) the lockfile is created by adding ".lock" to the
  * resolved path, and (2) upon commit, the resolved path is
diff --git a/merge.c b/merge.c
index 23866c9165..04ee5fc911 100644
--- a/merge.c
+++ b/merge.c
@@ -57,7 +57,7 @@ int checkout_fast_forward(const unsigned char *head,
 
refresh_cache(REFRESH_QUIET);
 
-   if (hold_locked_index(lock_file, 0) < 0)
+   if (hold_locked_index(lock_file, LOCK_REPORT_ON_ERROR) < 0)
return -1;
 
memset(, 0, sizeof(trees));
-- 
2.11.0-274-g0631465056

[PATCH 1/3] wt-status: implement opportunisitc index update correctly

[Junio C Hamano]

The require_clean_work_tree() function calls hold_locked_index()
with die_on_error=0 to signal that it is OK if it fails to obtain
the lock, but unconditionally calls update_index_if_able(), which
will try to write into fd=-1.

Signed-off-by: Junio C Hamano 
---
 wt-status.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index a2e9d332d8..a715e71906 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2258,11 +2258,12 @@ int has_uncommitted_changes(int ignore_submodules)
 int require_clean_work_tree(const char *action, const char *hint, int 
ignore_submodules, int gently)
 {
struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-   int err = 0;
+   int err = 0, fd;
 
-   hold_locked_index(lock_file, 0);
+   fd = hold_locked_index(lock_file, 0);
refresh_cache(REFRESH_QUIET);
-   update_index_if_able(_index, lock_file);
+   if (0 <= fd)
+   update_index_if_able(_index, lock_file);
rollback_lock_file(lock_file);
 
if (has_unstaged_changes(ignore_submodules)) {
-- 
2.11.0-274-g0631465056

[PATCH 2/3] hold_locked_index(): align error handling with hold_lockfile_for_update()

[Junio C Hamano]

Callers of the hold_locked_index() function pass 0 when they want to
prepare to write a new version of the index file without wishing to
die or emit an error message when the request fails (e.g. somebody
else already held the lock), and pass 1 when they want the call to
die upon failure.

This option is called LOCK_DIE_ON_ERROR by the underlying lockfile
API, and the hold_locked_index() function translates the paramter to
LOCK_DIE_ON_ERROR when calling the hold_lock_file_for_update().

Replace these hardcoded '1' with LOCK_DIE_ON_ERROR and stop
translating.  Callers other than the ones that are replaced with
this change pass '0' to the function; no behaviour change is
intended with this patch.

Signed-off-by: Junio C Hamano 
---

Among the callers of hold_locked_index() that passes 0:

 - diff.c::refresh_index_quietly() at the end of "git diff" is an
   opportunistic update; it leaks the lockfile structure but it is
   just before the program exits and nobody should care.

 - builtin/describe.c::cmd_describe(),
   builtin/commit.c::cmd_status(),
   sequencer.c::read_and_refresh_cache() are all opportunistic
   updates and they are OK.

 - builtin/update-index.c::cmd_update_index() takes a lock upfront
   but we may end up not needing to update the index (i.e. the
   entries may be fully up-to-date), in which case we do not need to
   issue an error upon failure to acquire the lock.  We do diagnose
   and die if we indeed need to update, so it is OK.

 - wt-status.c::require_clean_work_tree() IS BUGGY.  It asks
   silence, does not check the returned value.  Compare with
   callsites like cmd_describe() and cmd_status() to notice that it
   is wrong to call update_index_if_able() unconditionally.
---
 apply.c  | 2 +-
 builtin/add.c| 2 +-
 builtin/am.c | 6 +++---
 builtin/checkout-index.c | 2 +-
 builtin/checkout.c   | 4 ++--
 builtin/clone.c  | 2 +-
 builtin/commit.c | 8 
 builtin/merge.c  | 6 +++---
 builtin/mv.c | 2 +-
 builtin/read-tree.c  | 2 +-
 builtin/reset.c  | 2 +-
 builtin/rm.c | 2 +-
 builtin/update-index.c   | 1 +
 merge-recursive.c| 2 +-
 read-cache.c | 7 ++-
 rerere.c | 2 +-
 sequencer.c  | 2 +-
 t/helper/test-scrap-cache-tree.c | 2 +-
 18 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/apply.c b/apply.c
index 705cf562f0..2ed808d429 100644
--- a/apply.c
+++ b/apply.c
@@ -4688,7 +4688,7 @@ static int apply_patch(struct apply_state *state,
 
state->index_file,
 
LOCK_DIE_ON_ERROR);
else
-   state->newfd = hold_locked_index(state->lock_file, 1);
+   state->newfd = hold_locked_index(state->lock_file, 
LOCK_DIE_ON_ERROR);
}
 
if (state->check_index && read_apply_cache(state) < 0) {
diff --git a/builtin/add.c b/builtin/add.c
index e8fb80b36e..9f53f020d0 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -361,7 +361,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
add_new_files = !take_worktree_changes && !refresh_only;
require_pathspec = !(take_worktree_changes || (0 < addremove_explicit));
 
-   hold_locked_index(_file, 1);
+   hold_locked_index(_file, LOCK_DIE_ON_ERROR);
 
flags = ((verbose ? ADD_CACHE_VERBOSE : 0) |
 (show_only ? ADD_CACHE_PRETEND : 0) |
diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce9..bb5da422fc 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1119,7 +1119,7 @@ static void refresh_and_write_cache(void)
 {
struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
-   hold_locked_index(lock_file, 1);
+   hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
refresh_cache(REFRESH_QUIET);
if (write_locked_index(_index, lock_file, COMMIT_LOCK))
die(_("unable to write index file"));
@@ -1976,7 +1976,7 @@ static int fast_forward_to(struct tree *head, struct tree 
*remote, int reset)
return -1;
 
lock_file = xcalloc(1, sizeof(struct lock_file));
-   hold_locked_index(lock_file, 1);
+   hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
 
refresh_cache(REFRESH_QUIET);
 
@@ -2016,7 +2016,7 @@ static int merge_tree(struct tree *tree)
return -1;
 
lock_file = xcalloc(1, sizeof(struct lock_file));
-   hold_locked_index(lock_file, 1);
+   hold_locked_index(lock_file, LOCK_DIE_ON_ERROR);
 
memset(, 0, sizeof(opts));
opts.head_idx = 1;
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 30a49d9f42..07631d0c9c 100644
--- 

[PATCH 0/3] Do not be totally silent upon lock error

[Junio C Hamano]

Robbie Iannucci reported that "git merge" that fast-forwards fails
silently when a competing or stale index.lock is present in recent
Git:

$ git merge --ff-only master; echo $?
Updating 454cb6bd52..8d7a455ed5
1
$ exit

Did the update happen?  We used to give "fatal: Unable to create
..." followed by "Another git process seems to be running..."
advice, and that would have helped the user from the confusion.

This was because there were only two choices available to the
callers of the lockfile API--they can either ask it to die with
message when the lock cannot be acquired, or ask it to silently
return -1 to signal an error. The recent "libify sequencer" topic
turned many existing "please die" calls to "just silently return
-1", and while it added new "unable to lock" error messages to most
of them, one spot didn't get any and now is allowed to just die
silently.

This series should fix it:

 - 1/3 is something I noticed while reading the existing callers of
   the lockfile API, and is not a new issue with "libify sequencer"
   topic.

 - 2/3 gives a new third option to callers of the lockfile API; they
   can now say "please emit the usual error message upon failing to
   acquire the lock, but give control back to me".

 - 3/3 uses the new option to resurrect the message.

Junio C Hamano (3):
  wt-status: implement opportunisitc index update correctly
  hold_locked_index(): align error handling with hold_lockfile_for_update()
  lockfile: LOCK_REPORT_ON_ERROR

 apply.c  |  2 +-
 builtin/add.c|  2 +-
 builtin/am.c |  6 +++---
 builtin/checkout-index.c |  2 +-
 builtin/checkout.c   |  4 ++--
 builtin/clone.c  |  2 +-
 builtin/commit.c |  8 
 builtin/merge.c  |  6 +++---
 builtin/mv.c |  2 +-
 builtin/read-tree.c  |  2 +-
 builtin/reset.c  |  2 +-
 builtin/rm.c |  2 +-
 builtin/update-index.c   |  1 +
 lockfile.c   | 12 ++--
 lockfile.h   |  8 +++-
 merge-recursive.c|  2 +-
 merge.c  |  2 +-
 read-cache.c |  7 ++-
 rerere.c |  2 +-
 sequencer.c  |  2 +-
 t/helper/test-scrap-cache-tree.c |  2 +-
 wt-status.c  |  7 ---
 22 files changed, 49 insertions(+), 36 deletions(-)

-- 
2.11.0-274-g0631465056

[PATCH] submodule--helper: set alternateLocation for cloned submodules

[vi0oss]

From: Vitaly _Vi Shukela 

Git v2.11 introduced "git clone --recursive --referece ...",
but it didn't put the alternates for _nested_ submodules.

This patch makes all not just the root repository, but also
all submodules (recursively) have submodule.alternateLocation
and submodule.alternateErrorStrategy configured, making Git
search for possible alternates for nested submodules as well.

As submodule's alternate target does not end in .git/objects
(rather .git/modules/qq/objects), this alternate target
path restriction for in add_possible_reference_from_superproject
relates from "*.git/objects" to just */objects".

New tests have been added to t7408-submodule-reference.

Signed-off-by: Vitaly _Vi Shukela 
---
 builtin/submodule--helper.c| 24 --
 t/t7408-submodule-reference.sh | 73 ++
 2 files changed, 95 insertions(+), 2 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..93dae62 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -498,9 +498,9 @@ static int add_possible_reference_from_superproject(
 
/*
 * If the alternate object store is another repository, try the
-* standard layout with .git/modules//objects
+* standard layout with .git/(modules/)+/objects
 */
-   if (ends_with(alt->path, ".git/objects")) {
+   if (ends_with(alt->path, "/objects")) {
char *sm_alternate;
struct strbuf sb = STRBUF_INIT;
struct strbuf err = STRBUF_INIT;
@@ -672,6 +672,26 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
die(_("could not get submodule directory for '%s'"), path);
git_config_set_in_file(p, "core.worktree",
   relative_path(path, sm_gitdir, _path));
+
+   /* setup alternateLocation and alternateErrorStrategy in the cloned 
submodule if needed */
+   {
+   char *sm_alternate = NULL, *error_strategy = NULL;
+
+   git_config_get_string("submodule.alternateLocation", 
_alternate);
+   if (sm_alternate) {
+   git_config_set_in_file(p, "submodule.alternateLocation",
+  sm_alternate);
+   }
+   git_config_get_string("submodule.alternateErrorStrategy", 
_strategy);
+   if (error_strategy) {
+   git_config_set_in_file(p, 
"submodule.alternateErrorStrategy",
+  error_strategy);
+   }
+
+   free(sm_alternate);
+   free(error_strategy);
+   }
+
strbuf_release();
strbuf_release(_path);
free(sm_gitdir);
diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index 1c1e289..7b64725 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -125,4 +125,77 @@ test_expect_success 'ignoring missing submodule alternates 
passes clone and subm
)
 '
 
+test_expect_success 'preparing second superproject with a nested submodule' '
+   test_create_repo supersuper &&
+   (
+   cd supersuper &&
+   echo I am super super. >file &&
+   git add file &&
+   git commit -m B-super-super-initial
+   git submodule add "file://$base_dir/super" subwithsub &&
+   git commit -m B-super-super-added &&
+   git submodule update --init --recursive &&
+   git repack -ad
+   ) &&
+   echo not cleaning supersuper
+'
+
+# At this point there are three root-level positories: A, B, super and super2
+
+test_expect_success 'nested submodule alternate in works and is actually used' 
'
+   test_when_finished "rm -rf supersuper-clone" &&
+   git clone --recursive --reference supersuper supersuper 
supersuper-clone &&
+   (
+   cd supersuper-clone &&
+   # test superproject has alternates setup correctly
+   test_alternate_is_used .git/objects/info/alternates . &&
+   # immediate submodule has alternate:
+   test_alternate_is_used 
.git/modules/subwithsub/objects/info/alternates subwithsub
+   # nested submodule also has alternate:
+   test_alternate_is_used 
.git/modules/subwithsub/modules/sub/objects/info/alternates subwithsub/sub
+   )
+'
+
+test_expect_success 'missing nested submodule alternate fails clone and 
submodule update' '
+   test_when_finished "rm -rf supersuper-clone supersuper2" &&
+   git clone supersuper supersuper2 &&
+   (
+   cd supersuper2 &&
+   git submodule update --init
+   ) &&
+   test_must_fail git clone --recursive --reference supersuper2 
supersuper2 supersuper-clone &&
+   (
+   cd 

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Stephan Beyer]

Hi,

On 12/06/2016 07:58 PM, Junio C Hamano wrote:
> I was burned a few times with this in the past few years, but it did
> not irritate me often enough that I didn't write it down.  But I
> think this is serious enough that deserves attention from those who
> were involved.
> 
> A short reproduction recipe, using objects from git.git that are
> publicly available and stable, shows how bad it is.
> 
> $ git checkout v2.9.3^0
> 
> $ git cherry-pick 0582a34f52..a94bb68397
> ... see conflict, decide to give up backporting to
> ... such an old fork point.
> ... the git-prompt gives "|CHERRY-PICKING" correctly.
> 
> $ git reset --hard v2.10.2^0
> ... the git-prompt no longer says "|CHERRY-PICKING"
> 
> $ edit && git commit -m "prelim work for backporting"
> [detached HEAD cc5a6a9219] prelim work for backporting
> 
> $ git cherry-pick 0582a34f52..a94bb68397
> error: a cherry-pick or revert is already in progress
> hint: try "git cherry-pick (--continue | --quit | --abort)"
> fatal: cherry-pick failed
> 
> $ git cherry-pick --abort
> ... we come back to v2.9.3^0, losing the new commit!

Apart from the git-prompt bug: isn't this a user error? I think "git
cherry-pick --quit"[1] would be the right thing to do, not --abort.

On the other hand, one (as a user) could also expect that "git reset
--hard" also resets sequencer-related states (and that is what the
git-prompt suggests), but that would probably break a lot of scripts ;)

[1] By the way: git cherry-pick --quit, git rebase --forget ...
different wording for the same thing makes things unintuitive.

>  (1) The third invocation of "cherry-pick" with "--abort" to get rid
>  of the state from the unfinished cherry-pick we did previously
>  is necessary, but the command does not notice that we resetted
>  to a new branch AND we even did some other work there.  This
>  loses end-user's work.  
> 
>  "git cherry-pick --abort" should learn from "git am --abort"
>  that has an extra safety to deal with the above workflow.  The
>  state from the unfinished "am" is removed, but the head is not
>  rewound to avoid losing end-user's work.
> 
>  You can try by replacing two instances of
> 
>   $ git cherry-pick 0582a34f52..a94bb68397
> 
>  with
> 
>   $ git format-patch --stdout 0582a34f52..a94bb68397 | git am
> 
>  in the above sequence, and conclude with "git am--abort" to see
>  how much more pleasant and safe "git am --abort" is.
Definitely. I'd volunteer to add that safety guard. (But (2) remains.)

~Stephan

Re: Re* [BUG] Index.lock error message regression in git 2.11.0

[Junio C Hamano]

Junio C Hamano  writes:

> ...
> I would not be surprised if some existing calls to hold_lock*()
> functions that pass die_on_error=0 need to be updated to pass
> LOCK_SILENT_ON_ERROR, and when this fix is taken alone, it may look
> like a regression, but we are better off starting louder and squelch
> the ones that we find safe to make silent than the other way around.

I actually take this part back, for two reasons.

 * Before the recent js/sequencer-wo-die topic that made this
   failure mode of 'git merge' more dangerous by accident, there
   were already callers that passed die_on_error=0 to hold_lock*
   family of functions, and we can trust these callers.  They either
   have been silent upon lock failure sensibly (e.g. a caller that
   tries to acquire the lock to opportunistically update the index
   can safely choose not to do anything and be silent) or they have
   had their own way of reporting the errors to the users.  The "you
   need to ask to be totally quiet" approach in my rerolled patch
   (the one I am responding to) will introduce new regressions to
   these codepaths.

 * Among the ones that stopped passing die_on_error=1 when the topic
   was merged, there are
   ones that give sensible error messages.  Again, they do not need
   extra message with the "you need to ask to be totally quiet"
   approach [*1*].

We need to instead go through the latter, i.e. the ones that appear
in "git show --first-parent 2a4062a4a8", with fine-toothed comb to
see which 0 made an error totally silent (like the one Robbie
spotted in merge.c) and fix them to ask hold_lock*() functions not
to die but still report an error.

[PATCH 1/2] ref-filter, tag: eliminate duplicated sorting option parsing

[SZEDER Gábor]

Sorting options are either specified on the command line, which is
handled by parse_opt_ref_sorting() in ref-filter.c, or via the
'tag.sort' config variable, which is handled by parse_sorting_string()
in builtin/tag.c.  These two functions are nearly identical, the
difference being only their signature and the former having a couple
of extra lines at the beginning.

Eliminate the code duplication by making parse_sorting_string() part
of the public ref-filter API, and turning parse_opt_ref_sorting() into
a thin wrapper around that function.  This way builtin/tag.c can
continue using it as before (and it might be useful if there ever will
be a 'branch.sort' config variable).  Change its return type from int
to void, because it always returned zero and none of its callers cared
about it (the actual error handling is done inside
parse_ref_filter_atom() by die()ing on error).

Signed-off-by: SZEDER Gábor 
---
 builtin/tag.c | 24 
 ref-filter.c  | 16 +++-
 ref-filter.h  |  2 ++
 3 files changed, 13 insertions(+), 29 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 50e4ae567..6fe723bee 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -122,30 +122,6 @@ static const char tag_template_nocleanup[] =
"Lines starting with '%c' will be kept; you may remove them"
" yourself if you want to.\n");
 
-/* Parse arg given and add it the ref_sorting array */
-static int parse_sorting_string(const char *arg, struct ref_sorting 
**sorting_tail)
-{
-   struct ref_sorting *s;
-   int len;
-
-   s = xcalloc(1, sizeof(*s));
-   s->next = *sorting_tail;
-   *sorting_tail = s;
-
-   if (*arg == '-') {
-   s->reverse = 1;
-   arg++;
-   }
-   if (skip_prefix(arg, "version:", ) ||
-   skip_prefix(arg, "v:", ))
-   s->version = 1;
-
-   len = strlen(arg);
-   s->atom = parse_ref_filter_atom(arg, arg+len);
-
-   return 0;
-}
-
 static int git_tag_config(const char *var, const char *value, void *cb)
 {
int status;
diff --git a/ref-filter.c b/ref-filter.c
index bc551a752..dfadf577c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1667,15 +1667,11 @@ struct ref_sorting *ref_default_sorting(void)
return sorting;
 }
 
-int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
+void parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
 {
-   struct ref_sorting **sorting_tail = opt->value;
struct ref_sorting *s;
int len;
 
-   if (!arg) /* should --no-sort void the list ? */
-   return -1;
-
s = xcalloc(1, sizeof(*s));
s->next = *sorting_tail;
*sorting_tail = s;
@@ -1689,6 +1685,16 @@ int parse_opt_ref_sorting(const struct option *opt, 
const char *arg, int unset)
s->version = 1;
len = strlen(arg);
s->atom = parse_ref_filter_atom(arg, arg+len);
+}
+
+int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
+{
+   struct ref_sorting **sorting_tail = opt->value;
+
+   if (!arg) /* should --no-sort void the list ? */
+   return -1;
+
+   parse_sorting_string(arg, sorting_tail);
return 0;
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 14d435e2c..49466a17d 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -100,6 +100,8 @@ int verify_ref_format(const char *format);
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 /*  Print the ref using the given format and quote_style */
 void show_ref_array_item(struct ref_array_item *info, const char *format, int 
quote_style);
+/* Parse given arg and append it to the ref_sorting list */
+void parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail);
 /*  Callback function for parsing the sort option */
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int 
unset);
 /*  Default sort option based on refname */
-- 
2.11.0.78.g5a2d011

[PATCH 0/2] A bit of ref-filter atom parsing cleanups

[SZEDER Gábor]

The diffstat of the second patch doesn't show any benefits, but only
because the first patch removed a callsite that would have benefited from
it.

It merges cleanly with Karthik's "port branch.c to use ref-filter's
printing options" series.

SZEDER Gábor (2):
  ref-filter, tag: eliminate duplicated sorting option parsing
  ref-filter: add function to parse atoms from a nul-terminated string

 builtin/tag.c | 24 
 ref-filter.c  | 24 +---
 ref-filter.h  |  6 ++
 3 files changed, 19 insertions(+), 35 deletions(-)

-- 
2.11.0.78.g5a2d011

[PATCH 2/2] ref-filter: add function to parse atoms from a nul-terminated string

[SZEDER Gábor]

ref-filter's parse_ref_filter_atom() function parses an atom between
the start and end pointers it gets as arguments.  This is fine for two
of its callers, which process '%(atom)' format specifiers and the end
pointer comes directly from strchr() looking for the closing ')'.
However, it's not quite so straightforward for its other two callers,
which process sort specifiers given as plain nul-terminated strings.
Especially not for ref_default_sorting(), which has the default
hard-coded as a string literal, but can't use it directly, because a
pointer to the end of that string literal is needed as well.
The next patch will add yet another caller using a string literal.

Add a helper function around parse_ref_filter_atom() to parse an atom
up to the terminating nul, and call this helper in those two callers.

Signed-off-by: SZEDER Gábor 
---
 ref-filter.c | 8 ++--
 ref-filter.h | 4 
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index dfadf577c..3c6fd4ba7 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1658,19 +1658,16 @@ void show_ref_array_item(struct ref_array_item *info, 
const char *format, int qu
 /*  If no sorting option is given, use refname to sort as default */
 struct ref_sorting *ref_default_sorting(void)
 {
-   static const char cstr_name[] = "refname";
-
struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting));
 
sorting->next = NULL;
-   sorting->atom = parse_ref_filter_atom(cstr_name, cstr_name + 
strlen(cstr_name));
+   sorting->atom = parse_ref_filter_atom_from_string("refname");
return sorting;
 }
 
 void parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail)
 {
struct ref_sorting *s;
-   int len;
 
s = xcalloc(1, sizeof(*s));
s->next = *sorting_tail;
@@ -1683,8 +1680,7 @@ void parse_sorting_string(const char *arg, struct 
ref_sorting **sorting_tail)
if (skip_prefix(arg, "version:", ) ||
skip_prefix(arg, "v:", ))
s->version = 1;
-   len = strlen(arg);
-   s->atom = parse_ref_filter_atom(arg, arg+len);
+   s->atom = parse_ref_filter_atom_from_string(arg);
 }
 
 int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset)
diff --git a/ref-filter.h b/ref-filter.h
index 49466a17d..1250537cf 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -94,6 +94,10 @@ int filter_refs(struct ref_array *array, struct ref_filter 
*filter, unsigned int
 void ref_array_clear(struct ref_array *array);
 /*  Parse format string and sort specifiers */
 int parse_ref_filter_atom(const char *atom, const char *ep);
+static inline int parse_ref_filter_atom_from_string(const char *atom)
+{
+   return parse_ref_filter_atom(atom, atom+strlen(atom));
+}
 /*  Used to verify if the given format is correct and to parse out the used 
atoms */
 int verify_ref_format(const char *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
-- 
2.11.0.78.g5a2d011

Re: git repo vs project level authorization

[Sitaram Chamarty]

Ken,

On Mon, Dec 05, 2016 at 11:04:44PM +0100, Fredrik Gustafsson wrote:
> On Mon, Dec 05, 2016 at 03:33:51PM -0500, ken edward wrote:
> > I am currently using svn with apache+mod_dav_svn to have a single
> > repository with multiple projects. Each of the projects is controlled
> > by an access control file that lists the project path and the allowed
> > usernames.
> > 
> > Does git have this also? where is the doc?
> > 
> > Ken
> 
> Git does not do hosting or access control. For this you need to use a
> third party program. There are plenty of options for you and each has
> different features and limitations. For example you should take a look
> at gitolite, gitlab, bitbucket, github, gogs. Just to mention a few.
> It's also possible to setup git with ssh or http/https with your own
> access control methods. See the progit book for details here.

For some reason I did not see your email so I am responding to
Fredrik's.

If your current system is an access control file, gitolite may
be the closest "in spirit" to what you have;

The others that Fredrik mentioned are all much more GUI (and all
of them have additional features like issue tracking, code
reiew, etc.) If you need a more github-like experience, try
those out.

Gitolite does *only* access control, nothing else, but within
that limited scope it's pretty powerful.  The simplest/quickest
overview is probably this:

http://gitolite.com/gitolite/overview.html#basic-use-case

regards
sitaram

[PATCH v8 08/19] ref-filter: add support for %(upstream:track,nobracket)

[Karthik Nayak]

From: Karthik Nayak 

Add support for %(upstream:track,nobracket) which will print the
tracking information without the brackets (i.e. "ahead N, behind M").
This is needed when we port branch.c to use ref-filter's printing APIs.

Add test and documentation for the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 10 --
 ref-filter.c   | 67 +-
 t/t6300-for-each-ref.sh|  2 ++
 3 files changed, 53 insertions(+), 26 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 536846f..e1d250e 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -117,9 +117,13 @@ upstream::
`refname` above.  Additionally respects `:track` to show
"[ahead N, behind M]" and `:trackshort` to show the terse
version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
-   or "=" (in sync).  Has no effect if the ref does not have
-   tracking information associated with it. `:track` also prints
-   "[gone]" whenever unknown upstream ref is encountered.
+   or "=" (in sync). `:track` also prints "[gone]" whenever
+   unknown upstream ref is encountered. Append `:track,nobracket`
+   to show tracking information without brackets (i.e "ahead N,
+   behind M").  Has no effect if the ref does not have tracking
+   information associated with it.  All the options apart from
+   `nobracket` are mutually exclusive, but if used together the
+   last option is selected.
 
 push::
The name of a local ref which represents the `@{push}` location
diff --git a/ref-filter.c b/ref-filter.c
index e0229d3..49bb120 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -47,8 +47,10 @@ static struct used_atom {
union {
char color[COLOR_MAXLEN];
struct align align;
-   enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT }
-   remote_ref;
+   struct {
+   enum { RR_NORMAL, RR_SHORTEN, RR_TRACK, RR_TRACKSHORT } 
option;
+   unsigned int nobracket: 1;
+   } remote_ref;
struct {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB } option;
unsigned int nlines;
@@ -76,16 +78,33 @@ static void color_atom_parser(struct used_atom *atom, const 
char *color_value)
 
 static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 {
-   if (!arg)
-   atom->u.remote_ref = RR_NORMAL;
-   else if (!strcmp(arg, "short"))
-   atom->u.remote_ref = RR_SHORTEN;
-   else if (!strcmp(arg, "track"))
-   atom->u.remote_ref = RR_TRACK;
-   else if (!strcmp(arg, "trackshort"))
-   atom->u.remote_ref = RR_TRACKSHORT;
-   else
-   die(_("unrecognized format: %%(%s)"), atom->name);
+   struct string_list params = STRING_LIST_INIT_DUP;
+   int i;
+
+   if (!arg) {
+   atom->u.remote_ref.option = RR_NORMAL;
+   return;
+   }
+
+   atom->u.remote_ref.nobracket = 0;
+   string_list_split(, arg, ',', -1);
+
+   for (i = 0; i < params.nr; i++) {
+   const char *s = params.items[i].string;
+
+   if (!strcmp(s, "short"))
+   atom->u.remote_ref.option = RR_SHORTEN;
+   else if (!strcmp(s, "track"))
+   atom->u.remote_ref.option = RR_TRACK;
+   else if (!strcmp(s, "trackshort"))
+   atom->u.remote_ref.option = RR_TRACKSHORT;
+   else if (!strcmp(s, "nobracket"))
+   atom->u.remote_ref.nobracket = 1;
+   else
+   die(_("unrecognized format: %%(%s)"), atom->name);
+   }
+
+   string_list_clear(, 0);
 }
 
 static void body_atom_parser(struct used_atom *atom, const char *arg)
@@ -1049,25 +1068,27 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
struct branch *branch, const char **s)
 {
int num_ours, num_theirs;
-   if (atom->u.remote_ref == RR_SHORTEN)
+   if (atom->u.remote_ref.option == RR_SHORTEN)
*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
-   else if (atom->u.remote_ref == RR_TRACK) {
+   else if (atom->u.remote_ref.option == RR_TRACK) {
if (stat_tracking_info(branch, _ours,
   _theirs, NULL)) {
-   *s = "[gone]";
-   return;
-   }
-
-   if (!num_ours && !num_theirs)
+   *s = xstrdup("gone");
+

[PATCH v8 09/19] ref-filter: make "%(symref)" atom work with the ':short' modifier

[Karthik Nayak]

From: Karthik Nayak 

The "%(symref)" atom doesn't work when used with the ':short' modifier
because we strictly match only 'symref' for setting the 'need_symref'
indicator. Fix this by comparing with the valid_atom rather than the
used_atom.

Add tests for %(symref) and %(symref:short) while we're here.

Helped-by: Junio C Hamano 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c|  2 +-
 t/t6300-for-each-ref.sh | 24 
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/ref-filter.c b/ref-filter.c
index 49bb120..405a0e5 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -341,7 +341,7 @@ int parse_ref_filter_atom(const char *atom, const char *ep)
valid_atom[i].parser(_atom[at], arg);
if (*atom == '*')
need_tagged = 1;
-   if (!strcmp(used_atom[at].name, "symref"))
+   if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
return at;
 }
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index ed36a0a..1935583 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -38,6 +38,7 @@ test_atom() {
case "$1" in
head) ref=refs/heads/master ;;
 tag) ref=refs/tags/testtag ;;
+sym) ref=refs/heads/sym ;;
   *) ref=$1 ;;
esac
printf '%s\n' "$3" >expected
@@ -566,6 +567,7 @@ test_expect_success 'Verify sort with multiple keys' '
test_cmp expected actual
 '
 
+
 test_expect_success 'do not dereference NULL upon %(HEAD) on unborn branch' '
test_when_finished "git checkout master" &&
git for-each-ref --format="%(HEAD) %(refname:short)" refs/heads/ 
>actual &&
@@ -575,4 +577,26 @@ test_expect_success 'do not dereference NULL upon %(HEAD) 
on unborn branch' '
test_cmp expect actual
 '
 
+test_expect_success 'Add symbolic ref for the following tests' '
+   git symbolic-ref refs/heads/sym refs/heads/master
+'
+
+cat >expected expected 

[PATCH v8 03/19] ref-filter: implement %(if:equals=) and %(if:notequals=)

[Karthik Nayak]

From: Karthik Nayak 

Implement %(if:equals=) wherein the if condition is only
satisfied if the value obtained between the %(if:...) and %(then) atom
is the same as the given ''.

Similarly, implement (if:notequals=) wherein the if condition
is only satisfied if the value obtained between the %(if:...) and
%(then) atom is different from the given ''.

This is done by introducing 'if_atom_parser()' which parses the given
%(if) atom and then stores the data in used_atom which is later passed
on to the used_atom of the %(then) atom, so that it can do the required
comparisons.

Add tests and Documentation for the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt |  3 +++
 ref-filter.c   | 46 +-
 t/t6302-for-each-ref-filter.sh | 18 +++
 3 files changed, 62 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 3018969..392df6b 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -155,6 +155,9 @@ if::
evaluating the string before %(then), this is useful when we
use the %(HEAD) atom which prints either "*" or " " and we
want to apply the 'if' condition only on the 'HEAD' ref.
+   Append ":equals=" or ":notequals=" to compare
+   the value between the %(if:...) and %(then) atoms with the
+   given string.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index 5166326..7a21256 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -15,6 +15,7 @@
 #include "version.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
+typedef enum { COMPARE_EQUAL, COMPARE_UNEQUAL, COMPARE_NONE } cmp_status;
 
 struct align {
align_type position;
@@ -22,6 +23,8 @@ struct align {
 };
 
 struct if_then_else {
+   cmp_status cmp_status;
+   const char *str;
unsigned int then_atom_seen : 1,
else_atom_seen : 1,
condition_satisfied : 1;
@@ -49,6 +52,10 @@ static struct used_atom {
enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
C_SUB } option;
unsigned int nlines;
} contents;
+   struct {
+   cmp_status cmp_status;
+   const char *str;
+   } if_then_else;
enum { O_FULL, O_SHORT } objectname;
} u;
 } *used_atom;
@@ -169,6 +176,21 @@ static void align_atom_parser(struct used_atom *atom, 
const char *arg)
string_list_clear(, 0);
 }
 
+static void if_atom_parser(struct used_atom *atom, const char *arg)
+{
+   if (!arg) {
+   atom->u.if_then_else.cmp_status = COMPARE_NONE;
+   return;
+   } else if (skip_prefix(arg, "equals=", >u.if_then_else.str)) {
+   atom->u.if_then_else.cmp_status = COMPARE_EQUAL;
+   } else if (skip_prefix(arg, "notequals=", >u.if_then_else.str)) {
+   atom->u.if_then_else.cmp_status = COMPARE_UNEQUAL;
+   } else {
+   die(_("unrecognized %%(if) argument: %s"), arg);
+   }
+}
+
+
 static struct {
const char *name;
cmp_type cmp_type;
@@ -209,7 +231,7 @@ static struct {
{ "color", FIELD_STR, color_atom_parser },
{ "align", FIELD_STR, align_atom_parser },
{ "end" },
-   { "if" },
+   { "if", FIELD_STR, if_atom_parser },
{ "then" },
{ "else" },
 };
@@ -411,6 +433,9 @@ static void if_atom_handler(struct atom_value *atomv, 
struct ref_formatting_stat
struct ref_formatting_stack *new;
struct if_then_else *if_then_else = xcalloc(sizeof(struct 
if_then_else), 1);
 
+   if_then_else->str = atomv->atom->u.if_then_else.str;
+   if_then_else->cmp_status = atomv->atom->u.if_then_else.cmp_status;
+
push_stack_element(>stack);
new = state->stack;
new->at_end = if_then_else_handler;
@@ -442,10 +467,17 @@ static void then_atom_handler(struct atom_value *atomv, 
struct ref_formatting_st
die(_("format: %%(then) atom used after %%(else)"));
if_then_else->then_atom_seen = 1;
/*
-* If there exists non-empty string between the 'if' and
-* 'then' atom then the 'if' condition is satisfied.
+* If the 'equals' or 'notequals' attribute is used then
+* perform the required comparison. If not, only non-empty
+* strings satisfy the 'if' condition.
 */
-   if (cur->output.len && !is_empty(cur->output.buf))
+   if (if_then_else->cmp_status == COMPARE_EQUAL) {
+   if (!strcmp(if_then_else->str, cur->output.buf))
+  

[PATCH v8 06/19] ref-filter: introduce format_ref_array_item()

[Karthik Nayak]

From: Karthik Nayak 

To allow column display, we will need to first render the output in a
string list to allow print_columns() to compute the proper size of
each column before starting the actual output. Introduce the function
format_ref_array_item() that does the formatting of a ref_array_item
to an strbuf.

show_ref_array_item() is kept as a convenience wrapper around it which
obtains the strbuf and prints it the standard output.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 16 
 ref-filter.h |  3 +++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index bb69573..0f81f9f 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1799,10 +1799,10 @@ static void append_literal(const char *cp, const char 
*ep, struct ref_formatting
}
 }
 
-void show_ref_array_item(struct ref_array_item *info, const char *format, int 
quote_style)
+void format_ref_array_item(struct ref_array_item *info, const char *format,
+  int quote_style, struct strbuf *final_buf)
 {
const char *cp, *sp, *ep;
-   struct strbuf *final_buf;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
 
state.quote_style = quote_style;
@@ -1832,9 +1832,17 @@ void show_ref_array_item(struct ref_array_item *info, 
const char *format, int qu
}
if (state.stack->prev)
die(_("format: %%(end) atom missing"));
-   final_buf = >output;
-   fwrite(final_buf->buf, 1, final_buf->len, stdout);
+   strbuf_addbuf(final_buf, >output);
pop_stack_element();
+}
+
+void show_ref_array_item(struct ref_array_item *info, const char *format, int 
quote_style)
+{
+   struct strbuf final_buf = STRBUF_INIT;
+
+   format_ref_array_item(info, format, quote_style, _buf);
+   fwrite(final_buf.buf, 1, final_buf.len, stdout);
+   strbuf_release(_buf);
putchar('\n');
 }
 
diff --git a/ref-filter.h b/ref-filter.h
index 4aea594..0014b92 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -98,6 +98,9 @@ int parse_ref_filter_atom(const char *atom, const char *ep);
 int verify_ref_format(const char *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
+/*  Based on the given format and quote_style, fill the strbuf */
+void format_ref_array_item(struct ref_array_item *info, const char *format,
+  int quote_style, struct strbuf *final_buf);
 /*  Print the ref using the given format and quote_style */
 void show_ref_array_item(struct ref_array_item *info, const char *format, int 
quote_style);
 /*  Callback function for parsing the sort option */
-- 
2.10.2

[PATCH v8 01/19] ref-filter: implement %(if), %(then), and %(else) atoms

[Karthik Nayak]

From: Karthik Nayak 

Implement %(if), %(then) and %(else) atoms. Used as
%(if)...%(then)...%(end) or %(if)...%(then)...%(else)...%(end). If the
format string between %(if) and %(then) expands to an empty string, or
to only whitespaces, then the whole %(if)...%(end) expands to the string
following %(then). Otherwise, it expands to the string following
%(else), if any. Nesting of this construct is possible.

This is in preparation for porting over `git branch -l` to use
ref-filter APIs for printing.

Add Documentation and tests regarding the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt |  34 ++
 ref-filter.c   | 134 +++--
 t/t6302-for-each-ref-filter.sh |  76 +
 3 files changed, 237 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index f57e69b..3018969 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -146,6 +146,16 @@ align::
quoted, but if nested then only the topmost level performs
quoting.
 
+if::
+   Used as %(if)...%(then)...%(end) or
+   %(if)...%(then)...%(else)...%(end).  If there is an atom with
+   value or string literal after the %(if) then everything after
+   the %(then) is printed, else if the %(else) atom is used, then
+   everything after %(else) is printed. We ignore space when
+   evaluating the string before %(then), this is useful when we
+   use the %(HEAD) atom which prints either "*" or " " and we
+   want to apply the 'if' condition only on the 'HEAD' ref.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
@@ -181,6 +191,14 @@ As a special case for the date-type fields, you may 
specify a format for
 the date by adding `:` followed by date format name (see the
 values the `--date` option to linkgit:git-rev-list[1] takes).
 
+Some atoms like %(align) and %(if) always require a matching %(end).
+We call them "opening atoms" and sometimes denote them as %($open).
+
+When a scripting language specific quoting is in effect, everything
+between a top-level opening atom and its matching %(end) is evaluated
+according to the semantics of the opening atom and its result is
+quoted.
+
 
 EXAMPLES
 
@@ -268,6 +286,22 @@ eval=`git for-each-ref --shell --format="$fmt" \
 eval "$eval"
 
 
+
+An example to show the usage of %(if)...%(then)...%(else)...%(end).
+This prefixes the current branch with a star.
+
+
+git for-each-ref --format="%(if)%(HEAD)%(then)* %(else)  
%(end)%(refname:short)" refs/heads/
+
+
+
+An example to show the usage of %(if)...%(then)...%(end).
+This prints the authorname, if present.
+
+
+git for-each-ref --format="%(refname)%(if)%(authorname)%(then) 
%(color:red)Authored by: %(authorname)%(end)"
+
+
 SEE ALSO
 
 linkgit:git-show-ref[1]
diff --git a/ref-filter.c b/ref-filter.c
index f5f7a70..2fed7fe 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -21,6 +21,12 @@ struct align {
unsigned int width;
 };
 
+struct if_then_else {
+   unsigned int then_atom_seen : 1,
+   else_atom_seen : 1,
+   condition_satisfied : 1;
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -203,6 +209,9 @@ static struct {
{ "color", FIELD_STR, color_atom_parser },
{ "align", FIELD_STR, align_atom_parser },
{ "end" },
+   { "if" },
+   { "then" },
+   { "else" },
 };
 
 #define REF_FORMATTING_STATE_INIT  { 0, NULL }
@@ -210,7 +219,7 @@ static struct {
 struct ref_formatting_stack {
struct ref_formatting_stack *prev;
struct strbuf output;
-   void (*at_end)(struct ref_formatting_stack *stack);
+   void (*at_end)(struct ref_formatting_stack **stack);
void *at_end_data;
 };
 
@@ -343,13 +352,14 @@ static void pop_stack_element(struct ref_formatting_stack 
**stack)
*stack = prev;
 }
 
-static void end_align_handler(struct ref_formatting_stack *stack)
+static void end_align_handler(struct ref_formatting_stack **stack)
 {
-   struct align *align = (struct align *)stack->at_end_data;
+   struct ref_formatting_stack *cur = *stack;
+   struct align *align = (struct align *)cur->at_end_data;
struct strbuf s = STRBUF_INIT;
 
-   strbuf_utf8_align(, align->position, align->width, stack->output.buf);
-   strbuf_swap(>output, );
+   strbuf_utf8_align(, align->position, align->width, cur->output.buf);
+   strbuf_swap(>output, );
strbuf_release();
 }
 
@@ -363,6 

[PATCH v8 05/19] ref-filter: move get_head_description() from branch.c

[Karthik Nayak]

From: Karthik Nayak 

Move the implementation of get_head_description() from branch.c to
ref-filter.  This gives a description of the HEAD ref if called. This
is used as the refname for the HEAD ref whenever the
FILTER_REFS_DETACHED_HEAD option is used. Make it public because we
need it to calculate the length of the HEAD refs description in
branch.c:calc_maxwidth() when we port branch.c to use ref-filter
APIs.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 builtin/branch.c | 33 -
 ref-filter.c | 38 --
 ref-filter.h |  2 ++
 3 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 60cc5c8..0b80c13 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -364,39 +364,6 @@ static void add_verbose_info(struct strbuf *out, struct 
ref_array_item *item,
strbuf_release();
 }
 
-static char *get_head_description(void)
-{
-   struct strbuf desc = STRBUF_INIT;
-   struct wt_status_state state;
-   memset(, 0, sizeof(state));
-   wt_status_get_state(, 1);
-   if (state.rebase_in_progress ||
-   state.rebase_interactive_in_progress)
-   strbuf_addf(, _("(no branch, rebasing %s)"),
-   state.branch);
-   else if (state.bisect_in_progress)
-   strbuf_addf(, _("(no branch, bisect started on %s)"),
-   state.branch);
-   else if (state.detached_from) {
-   if (state.detached_at)
-   /* TRANSLATORS: make sure this matches
-  "HEAD detached at " in wt-status.c */
-   strbuf_addf(, _("(HEAD detached at %s)"),
-   state.detached_from);
-   else
-   /* TRANSLATORS: make sure this matches
-  "HEAD detached from " in wt-status.c */
-   strbuf_addf(, _("(HEAD detached from %s)"),
-   state.detached_from);
-   }
-   else
-   strbuf_addstr(, _("(no branch)"));
-   free(state.branch);
-   free(state.onto);
-   free(state.detached_from);
-   return strbuf_detach(, NULL);
-}
-
 static void format_and_print_ref_item(struct ref_array_item *item, int 
maxwidth,
  struct ref_filter *filter, const char 
*remote_prefix)
 {
diff --git a/ref-filter.c b/ref-filter.c
index d1534d0..bb69573 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -13,6 +13,7 @@
 #include "utf8.h"
 #include "git-compat-util.h"
 #include "version.h"
+#include "wt-status.h"
 
 typedef enum { FIELD_STR, FIELD_ULONG, FIELD_TIME } cmp_type;
 typedef enum { COMPARE_EQUAL, COMPARE_UNEQUAL, COMPARE_NONE } cmp_status;
@@ -1081,6 +1082,37 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
*s = refname;
 }
 
+char *get_head_description(void)
+{
+   struct strbuf desc = STRBUF_INIT;
+   struct wt_status_state state;
+   memset(, 0, sizeof(state));
+   wt_status_get_state(, 1);
+   if (state.rebase_in_progress ||
+   state.rebase_interactive_in_progress)
+   strbuf_addf(, _("(no branch, rebasing %s)"),
+   state.branch);
+   else if (state.bisect_in_progress)
+   strbuf_addf(, _("(no branch, bisect started on %s)"),
+   state.branch);
+   else if (state.detached_from) {
+   /* TRANSLATORS: make sure these match _("HEAD detached at ")
+  and _("HEAD detached from ") in wt-status.c */
+   if (state.detached_at)
+   strbuf_addf(, _("(HEAD detached at %s)"),
+   state.detached_from);
+   else
+   strbuf_addf(, _("(HEAD detached from %s)"),
+   state.detached_from);
+   }
+   else
+   strbuf_addstr(, _("(no branch)"));
+   free(state.branch);
+   free(state.onto);
+   free(state.detached_from);
+   return strbuf_detach(, NULL);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -1120,9 +1152,11 @@ static void populate_value(struct ref_array_item *ref)
name++;
}
 
-   if (starts_with(name, "refname"))
+   if (starts_with(name, "refname")) {
refname = ref->refname;
-   else if (starts_with(name, "symref"))
+   if (ref->kind & FILTER_REFS_DETACHED_HEAD)
+   refname = get_head_description();
+   } else if (starts_with(name, "symref"))
refname = ref->symref ? ref->symref : "";
   

[PATCH v8 04/19] ref-filter: modify "%(objectname:short)" to take length

[Karthik Nayak]

From: Karthik Nayak 

Add support for %(objectname:short=) which would print the
abbreviated unique objectname of given length. When no length is
specified, the length is 'DEFAULT_ABBREV'. The minimum length is
'MINIMUM_ABBREV'. The length may be exceeded to ensure that the
provided object name is unique.

Add tests and documentation for the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Helped-by: Jacob Keller 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt |  3 +++
 ref-filter.c   | 25 +++--
 t/t6300-for-each-ref.sh| 10 ++
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 392df6b..b730735 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -107,6 +107,9 @@ objectsize::
 objectname::
The object name (aka SHA-1).
For a non-ambiguous abbreviation of the object name append `:short`.
+   For an abbreviation of the object name with desired length append
+   `:short=`, where the minimum length is MINIMUM_ABBREV. The
+   length may be exceeded to ensure unique object names.
 
 upstream::
The name of a local ref which can be considered ``upstream''
diff --git a/ref-filter.c b/ref-filter.c
index 7a21256..d1534d0 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -56,7 +56,10 @@ static struct used_atom {
cmp_status cmp_status;
const char *str;
} if_then_else;
-   enum { O_FULL, O_SHORT } objectname;
+   struct {
+   enum { O_FULL, O_LENGTH, O_SHORT } option;
+   unsigned int length;
+   } objectname;
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -119,10 +122,17 @@ static void contents_atom_parser(struct used_atom *atom, 
const char *arg)
 static void objectname_atom_parser(struct used_atom *atom, const char *arg)
 {
if (!arg)
-   atom->u.objectname = O_FULL;
+   atom->u.objectname.option = O_FULL;
else if (!strcmp(arg, "short"))
-   atom->u.objectname = O_SHORT;
-   else
+   atom->u.objectname.option = O_SHORT;
+   else if (skip_prefix(arg, "short=", )) {
+   atom->u.objectname.option = O_LENGTH;
+   if (strtoul_ui(arg, 10, >u.objectname.length) ||
+   atom->u.objectname.length == 0)
+   die(_("positive value expected objectname:short=%s"), 
arg);
+   if (atom->u.objectname.length < MINIMUM_ABBREV)
+   atom->u.objectname.length = MINIMUM_ABBREV;
+   } else
die(_("unrecognized %%(objectname) argument: %s"), arg);
 }
 
@@ -595,12 +605,15 @@ static int grab_objectname(const char *name, const 
unsigned char *sha1,
   struct atom_value *v, struct used_atom *atom)
 {
if (starts_with(name, "objectname")) {
-   if (atom->u.objectname == O_SHORT) {
+   if (atom->u.objectname.option == O_SHORT) {
v->s = xstrdup(find_unique_abbrev(sha1, 
DEFAULT_ABBREV));
return 1;
-   } else if (atom->u.objectname == O_FULL) {
+   } else if (atom->u.objectname.option == O_FULL) {
v->s = xstrdup(sha1_to_hex(sha1));
return 1;
+   } else if (atom->u.objectname.option == O_LENGTH) {
+   v->s = xstrdup(find_unique_abbrev(sha1, 
atom->u.objectname.length));
+   return 1;
} else
die("BUG: unknown %%(objectname) option");
}
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 039509a..644f169 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -60,6 +60,8 @@ test_atom head objecttype commit
 test_atom head objectsize 171
 test_atom head objectname $(git rev-parse refs/heads/master)
 test_atom head objectname:short $(git rev-parse --short refs/heads/master)
+test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
+test_atom head objectname:short=10 $(git rev-parse --short=10 
refs/heads/master)
 test_atom head tree $(git rev-parse refs/heads/master^{tree})
 test_atom head parent ''
 test_atom head numparent 0
@@ -99,6 +101,8 @@ test_atom tag objecttype tag
 test_atom tag objectsize 154
 test_atom tag objectname $(git rev-parse refs/tags/testtag)
 test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag)
+test_atom head objectname:short=1 $(git rev-parse --short=1 refs/heads/master)
+test_atom head objectname:short=10 $(git rev-parse --short=10 
refs/heads/master)
 

[PATCH v8 19/19] branch: implement '--format' option

[Karthik Nayak]

From: Karthik Nayak 

Implement the '--format' option provided by 'ref-filter'. This lets the
user list branches as per desired format similar to the implementation
in 'git for-each-ref'.

Add tests and documentation for the same.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-branch.txt |  7 ++-
 builtin/branch.c | 14 +-
 t/t3203-branch-output.sh | 14 ++
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 1fe7344..e5b6f31 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -12,7 +12,7 @@ SYNOPSIS
[--list] [-v [--abbrev= | --no-abbrev]]
[--column[=] | --no-column]
[(--merged | --no-merged | --contains) []] [--sort=]
-   [--points-at ] [...]
+   [--points-at ] [--format=] [...]
 'git branch' [--set-upstream | --track | --no-track] [-l] [-f]  
[]
 'git branch' (--set-upstream-to= | -u ) []
 'git branch' --unset-upstream []
@@ -246,6 +246,11 @@ start-point is either a local or remote-tracking branch.
 --points-at ::
Only list branches of the given object.
 
+--format ::
+   A string that interpolates `%(fieldname)` from the object
+   pointed at by a ref being shown.  The format is the same as
+   that of linkgit:git-for-each-ref[1].
+
 Examples
 
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 046d245..6393c3c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -28,6 +28,7 @@ static const char * const builtin_branch_usage[] = {
N_("git branch [] [-r] (-d | -D) ..."),
N_("git branch [] (-m | -M) [] "),
N_("git branch [] [-r | -a] [--points-at]"),
+   N_("git branch [] [-r | -a] [--format]"),
NULL
 };
 
@@ -364,14 +365,14 @@ static char *build_format(struct ref_filter *filter, int 
maxwidth, const char *r
return strbuf_detach(, NULL);
 }
 
-static void print_ref_list(struct ref_filter *filter, struct ref_sorting 
*sorting)
+static void print_ref_list(struct ref_filter *filter, struct ref_sorting 
*sorting, const char *format)
 {
int i;
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
struct strbuf out = STRBUF_INIT;
-   char *format;
+   char *to_free = NULL;
 
/*
 * If we are listing more than just remote branches,
@@ -388,7 +389,8 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
if (filter->verbose)
maxwidth = calc_maxwidth(, strlen(remote_prefix));
 
-   format = build_format(filter, maxwidth, remote_prefix);
+   if (!format)
+   format = to_free = build_format(filter, maxwidth, 
remote_prefix);
verify_ref_format(format);
 
/*
@@ -416,7 +418,7 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
}
 
ref_array_clear();
-   free(format);
+   free(to_free);
 }
 
 static void reject_rebase_or_bisect_branch(const char *target)
@@ -536,6 +538,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
enum branch_track track;
struct ref_filter filter;
static struct ref_sorting *sorting = NULL, **sorting_tail = 
+   const char *format = NULL;
 
struct option options[] = {
OPT_GROUP(N_("Generic options")),
@@ -576,6 +579,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPTION_CALLBACK, 0, "points-at", _at, 
N_("object"),
N_("print only branches of the object"), 0, 
parse_opt_object_name
},
+   OPT_STRING(  0 , "format", , N_("format"), N_("format to 
use for the output")),
OPT_END(),
};
 
@@ -636,7 +640,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if ((filter.kind & FILTER_REFS_BRANCHES) && filter.detached)
filter.kind |= FILTER_REFS_DETACHED_HEAD;
filter.name_patterns = argv;
-   print_ref_list(, sorting);
+   print_ref_list(, sorting, format);
print_columns(, colopts, NULL);
string_list_clear(, 0);
return 0;
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 980c732..d8edaf2 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -196,4 +196,18 @@ test_expect_success 'local-branch symrefs shortened 
properly' '
test_cmp expect actual
 '
 
+test_expect_success 'git branch --format option' '
+   cat >expect <<-\EOF &&
+   Refname is (HEAD detached from fromtag)
+   Refname is refs/heads/ambiguous
+   Refname is refs/heads/branch-one
+   Refname is 

[PATCH v8 18/19] branch: use ref-filter printing APIs

[Karthik Nayak]

From: Karthik Nayak 

Port branch.c to use ref-filter APIs for printing. This clears out
most of the code used in branch.c for printing and replaces them with
calls made to the ref-filter library.

Introduce build_format() which gets the format required for printing
of refs. Make amendments to print_ref_list() to reflect these changes.

The strings included in build_format() may not be safely quoted for
inclusion (i.e. it might contain '%' which needs to be escaped with an
additional '%'). Introduce quote_literal_for_format() as a helper
function which takes a string and returns a version of the string that
is safely quoted to be used in the for-each-ref format which is built
in build_format().

Change calc_maxwidth() to also account for the length of HEAD ref, by
calling ref-filter:get_head_discription().

Also change the test in t6040 to reflect the changes.

Before this patch, all cross-prefix symrefs weren't shortened. Since
we're using ref-filter APIs, we shorten all symrefs by default. We also
allow the user to change the format if needed with the introduction of
the '--format' option in the next patch.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Helped-by: Junio C Hamano 
Signed-off-by: Karthik Nayak 
---
 builtin/branch.c | 249 ---
 t/t3203-branch-output.sh |   2 +-
 t/t6040-tracking-info.sh |   2 +-
 3 files changed, 88 insertions(+), 165 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4d06553..046d245 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -36,12 +36,12 @@ static unsigned char head_sha1[20];
 
 static int branch_use_color = -1;
 static char branch_colors[][COLOR_MAXLEN] = {
-   GIT_COLOR_RESET,
-   GIT_COLOR_NORMAL,   /* PLAIN */
-   GIT_COLOR_RED,  /* REMOTE */
-   GIT_COLOR_NORMAL,   /* LOCAL */
-   GIT_COLOR_GREEN,/* CURRENT */
-   GIT_COLOR_BLUE, /* UPSTREAM */
+   "%(color:reset)",
+   "%(color:reset)",   /* PLAIN */
+   "%(color:red)", /* REMOTE */
+   "%(color:reset)",   /* LOCAL */
+   "%(color:green)",   /* CURRENT */
+   "%(color:blue)",/* UPSTREAM */
 };
 enum color_branch {
BRANCH_COLOR_RESET = 0,
@@ -280,180 +280,88 @@ static int delete_branches(int argc, const char **argv, 
int force, int kinds,
return(ret);
 }
 
-static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
-   int show_upstream_ref)
+static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
 {
-   int ours, theirs;
-   char *ref = NULL;
-   struct branch *branch = branch_get(branch_name);
-   const char *upstream;
-   struct strbuf fancy = STRBUF_INIT;
-   int upstream_is_gone = 0;
-   int added_decoration = 1;
-
-   if (stat_tracking_info(branch, , , ) < 0) {
-   if (!upstream)
-   return;
-   upstream_is_gone = 1;
-   }
-
-   if (show_upstream_ref) {
-   ref = shorten_unambiguous_ref(upstream, 0);
-   if (want_color(branch_use_color))
-   strbuf_addf(, "%s%s%s",
-   branch_get_color(BRANCH_COLOR_UPSTREAM),
-   ref, 
branch_get_color(BRANCH_COLOR_RESET));
-   else
-   strbuf_addstr(, ref);
-   }
+   int i, max = 0;
+   for (i = 0; i < refs->nr; i++) {
+   struct ref_array_item *it = refs->items[i];
+   const char *desc = it->refname;
+   int w;
 
-   if (upstream_is_gone) {
-   if (show_upstream_ref)
-   strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
-   else
-   added_decoration = 0;
-   } else if (!ours && !theirs) {
-   if (show_upstream_ref)
-   strbuf_addf(stat, _("[%s]"), fancy.buf);
-   else
-   added_decoration = 0;
-   } else if (!ours) {
-   if (show_upstream_ref)
-   strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, 
theirs);
-   else
-   strbuf_addf(stat, _("[behind %d]"), theirs);
+   skip_prefix(it->refname, "refs/heads/", );
+   skip_prefix(it->refname, "refs/remotes/", );
+   if (it->kind == FILTER_REFS_DETACHED_HEAD) {
+   char *head_desc = get_head_description();
+   w = utf8_strwidth(head_desc);
+   free(head_desc);
+   } else
+   w = utf8_strwidth(desc);
 
-   } else if (!theirs) {
-   if (show_upstream_ref)
-   strbuf_addf(stat, _("[%s: ahead %d]"), fancy.buf, ours);
-   

[PATCH v8 17/19] branch, tag: use porcelain output

[Karthik Nayak]

From: Karthik Nayak 

Call ref-filter's setup_ref_filter_porcelain_msg() to enable
translated messages for the %(upstream:tack) atom. Although branch.c
doesn't currently use ref-filter's printing API's, this will ensure
that when it does in the future patches, we do not need to worry about
translation.

Written-by: Matthieu Moy 
Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Signed-off-by: Karthik Nayak 
---
 builtin/branch.c | 2 ++
 builtin/tag.c| 2 ++
 2 files changed, 4 insertions(+)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0b80c13..4d06553 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -656,6 +656,8 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
OPT_END(),
};
 
+   setup_ref_filter_porcelain_msg();
+
memset(, 0, sizeof(filter));
filter.kind = FILTER_REFS_BRANCHES;
filter.abbrev = -1;
diff --git a/builtin/tag.c b/builtin/tag.c
index 24bbe24..774e955 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -373,6 +373,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_END()
};
 
+   setup_ref_filter_porcelain_msg();
+
git_config(git_tag_config, sorting_tail);
 
memset(, 0, sizeof(opt));
-- 
2.10.2

[PATCH v8 07/19] ref-filter: make %(upstream:track) prints "[gone]" for invalid upstreams

[Karthik Nayak]

From: Karthik Nayak 

Borrowing from branch.c's implementation print "[gone]" whenever an
unknown upstream ref is encountered instead of just ignoring it.

This makes sure that when branch.c is ported over to using ref-filter
APIs for printing, this feature is not lost.

Make changes to t/t6300-for-each-ref.sh and
Documentation/git-for-each-ref.txt to reflect this change.

Mentored-by: Christian Couder 
Mentored-by: Matthieu Moy 
Helped-by : Jacob Keller 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 3 ++-
 ref-filter.c   | 4 +++-
 t/t6300-for-each-ref.sh| 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index b730735..536846f 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -118,7 +118,8 @@ upstream::
"[ahead N, behind M]" and `:trackshort` to show the terse
version: ">" (ahead), "<" (behind), "<>" (ahead and behind),
or "=" (in sync).  Has no effect if the ref does not have
-   tracking information associated with it.
+   tracking information associated with it. `:track` also prints
+   "[gone]" whenever unknown upstream ref is encountered.
 
 push::
The name of a local ref which represents the `@{push}` location
diff --git a/ref-filter.c b/ref-filter.c
index 0f81f9f..e0229d3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1053,8 +1053,10 @@ static void fill_remote_ref_details(struct used_atom 
*atom, const char *refname,
*s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
else if (atom->u.remote_ref == RR_TRACK) {
if (stat_tracking_info(branch, _ours,
-  _theirs, NULL))
+  _theirs, NULL)) {
+   *s = "[gone]";
return;
+   }
 
if (!num_ours && !num_theirs)
*s = "";
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 644f169..5019f40 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -382,7 +382,7 @@ test_expect_success 'Check that :track[short] cannot be 
used with other atoms' '
 
 test_expect_success 'Check that :track[short] works when upstream is invalid' '
cat >expected <<-\EOF &&
-
+   [gone]
 
EOF
test_when_finished "git config branch.master.merge refs/heads/master" &&
-- 
2.10.2

[PATCH v8 13/19] ref-filter: rename the 'strip' option to 'lstrip'

[Karthik Nayak]

In preparation for the upcoming patch, where we introduce the 'rstrip'
option. Rename the 'strip' option to 'lstrip' to remove ambiguity.

Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 10 +-
 builtin/tag.c  |  4 ++--
 ref-filter.c   | 20 ++--
 t/t6300-for-each-ref.sh| 22 +++---
 4 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 6a1e747..f85ccff 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -92,9 +92,9 @@ refname::
The name of the ref (the part after $GIT_DIR/).
For a non-ambiguous short name of the ref append `:short`.
The option core.warnAmbiguousRefs is used to select the strict
-   abbreviation mode. If `strip=` is appended, strips ``
+   abbreviation mode. If `lstrip=` is appended, strips ``
slash-separated path components from the front of the refname
-   (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
+   (e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo`.
`` must be a positive integer.  If a displayed ref has fewer
components than ``, the command aborts with an error.
 
@@ -113,7 +113,7 @@ objectname::
 
 upstream::
The name of a local ref which can be considered ``upstream''
-   from the displayed ref. Respects `:short` and `:strip` in the
+   from the displayed ref. Respects `:short` and `:lstrip` in the
same way as `refname` above.  Additionally respects `:track`
to show "[ahead N, behind M]" and `:trackshort` to show the
terse version: ">" (ahead), "<" (behind), "<>" (ahead and
@@ -127,7 +127,7 @@ upstream::
 
 push::
The name of a local ref which represents the `@{push}`
-   location for the displayed ref. Respects `:short`, `:strip`,
+   location for the displayed ref. Respects `:short`, `:lstrip`,
`:track`, and `:trackshort` options as `upstream`
does. Produces an empty string if no `@{push}` ref is
configured.
@@ -171,7 +171,7 @@ if::
 symref::
The ref which the given symbolic ref refers to. If not a
symbolic ref, nothing is printed. Respects the `:short` and
-   `:strip` options in the same way as `refname` above.
+   `:lstrip` options in the same way as `refname` above.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/builtin/tag.c b/builtin/tag.c
index 50e4ae5..24bbe24 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -45,11 +45,11 @@ static int list_tags(struct ref_filter *filter, struct 
ref_sorting *sorting, con
if (!format) {
if (filter->lines) {
to_free = xstrfmt("%s %%(contents:lines=%d)",
- "%(align:15)%(refname:strip=2)%(end)",
+ 
"%(align:15)%(refname:lstrip=2)%(end)",
  filter->lines);
format = to_free;
} else
-   format = "%(refname:strip=2)";
+   format = "%(refname:lstrip=2)";
}
 
verify_ref_format(format);
diff --git a/ref-filter.c b/ref-filter.c
index be535a6..c74b08d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -32,8 +32,8 @@ struct if_then_else {
 };
 
 struct refname_atom {
-   enum { R_NORMAL, R_SHORT, R_STRIP } option;
-   unsigned int strip;
+   enum { R_NORMAL, R_SHORT, R_LSTRIP } option;
+   unsigned int lstrip;
 };
 
 /*
@@ -90,10 +90,10 @@ static void refname_atom_parser_internal(struct 
refname_atom *atom,
atom->option = R_NORMAL;
else if (!strcmp(arg, "short"))
atom->option = R_SHORT;
-   else if (skip_prefix(arg, "strip=", )) {
-   atom->option = R_STRIP;
-   if (strtoul_ui(arg, 10, >strip) || atom->strip <= 0)
-   die(_("positive value expected refname:strip=%s"), arg);
+   else if (skip_prefix(arg, "lstrip=", )) {
+   atom->option = R_LSTRIP;
+   if (strtoul_ui(arg, 10, >lstrip) || atom->lstrip <= 0)
+   die(_("positive value expected refname:lstrip=%s"), 
arg);
} else
die(_("unrecognized %%(%s) argument: %s"), name, arg);
 }
@@ -1071,7 +1071,7 @@ static inline char *copy_advance(char *dst, const char 
*src)
return dst;
 }
 
-static const char *strip_ref_components(const char *refname, unsigned int len)
+static const char *lstrip_ref_components(const char *refname, unsigned int len)
 {
long remaining = len;
const char *start = refname;
@@ -1079,7 +1079,7 @@ static const char *strip_ref_components(const char 
*refname, unsigned int len)
 

[PATCH v8 14/19] ref-filter: modify the 'lstrip=' option to work with negative ''

[Karthik Nayak]

Currently the 'lstrip=' option only takes a positive value ''
and strips '' slash-separated path components from the left. Modify
the 'lstrip' option to also take a negative number '' which would
only _leave_ behind 'N' slash-separated path components from the left.

Add documentation and tests for the same.

Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt |  5 +++--
 ref-filter.c   | 26 +-
 t/t6300-for-each-ref.sh| 15 ---
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index f85ccff..ad9b243 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -95,8 +95,9 @@ refname::
abbreviation mode. If `lstrip=` is appended, strips ``
slash-separated path components from the front of the refname
(e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo`.
-   `` must be a positive integer.  If a displayed ref has fewer
-   components than ``, the command aborts with an error.
+   if `` is a negative number, then only `` path components
+   are left behind.  If a displayed ref has fewer components than
+   ``, the command aborts with an error.
 
 objecttype::
The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/ref-filter.c b/ref-filter.c
index c74b08d..deb2b29 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -33,7 +33,7 @@ struct if_then_else {
 
 struct refname_atom {
enum { R_NORMAL, R_SHORT, R_LSTRIP } option;
-   unsigned int lstrip;
+   int lstrip;
 };
 
 /*
@@ -92,8 +92,8 @@ static void refname_atom_parser_internal(struct refname_atom 
*atom,
atom->option = R_SHORT;
else if (skip_prefix(arg, "lstrip=", )) {
atom->option = R_LSTRIP;
-   if (strtoul_ui(arg, 10, >lstrip) || atom->lstrip <= 0)
-   die(_("positive value expected refname:lstrip=%s"), 
arg);
+   if (strtol_i(arg, 10, >lstrip))
+   die(_("Integer value expected refname:lstrip=%s"), arg);
} else
die(_("unrecognized %%(%s) argument: %s"), name, arg);
 }
@@ -1071,21 +1071,37 @@ static inline char *copy_advance(char *dst, const char 
*src)
return dst;
 }
 
-static const char *lstrip_ref_components(const char *refname, unsigned int len)
+static const char *lstrip_ref_components(const char *refname, int len)
 {
long remaining = len;
const char *start = refname;
 
+   if (len < 0) {
+   int i;
+   const char *p = refname;
+
+   /* Find total no of '/' separated path-components */
+   for (i = 0; p[i]; p[i] == '/' ? i++ : *p++);
+   /*
+* The number of components we need to strip is now
+* the total minus the components to be left (Plus one
+* because we count the number of '/', but the number
+* of components is one more than the no of '/').
+*/
+   remaining = i + len + 1;
+   }
+
while (remaining) {
switch (*start++) {
case '\0':
-   die(_("ref '%s' does not have %ud components to 
:lstrip"),
+   die(_("ref '%s' does not have %d components to 
:lstrip"),
refname, len);
case '/':
remaining--;
break;
}
}
+
return start;
 }
 
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2b1af9c..26adca8 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -53,12 +53,16 @@ test_atom head refname refs/heads/master
 test_atom head refname:short master
 test_atom head refname:lstrip=1 heads/master
 test_atom head refname:lstrip=2 master
+test_atom head refname:lstrip=-1 master
+test_atom head refname:lstrip=-2 heads/master
 test_atom head upstream refs/remotes/origin/master
 test_atom head upstream:short origin/master
 test_atom head upstream:lstrip=2 origin/master
+test_atom head upstream:lstrip=-2 origin/master
 test_atom head push refs/remotes/myfork/master
 test_atom head push:short myfork/master
 test_atom head push:lstrip=1 remotes/myfork/master
+test_atom head push:lstrip=-1 master
 test_atom head objecttype commit
 test_atom head objectsize 171
 test_atom head objectname $(git rev-parse refs/heads/master)
@@ -141,14 +145,9 @@ test_expect_success 'Check invalid atoms names are errors' 
'
test_must_fail git for-each-ref --format="%(INVALID)" refs/heads
 '
 
-test_expect_success 'arguments to :lstrip must be positive integers' '
-   test_must_fail git for-each-ref --format="%(refname:lstrip=0)" &&
-   test_must_fail git for-each-ref --format="%(refname:lstrip=-1)" &&
-   test_must_fail 

[PATCH v8 10/19] ref-filter: introduce refname_atom_parser_internal()

[Karthik Nayak]

From: Karthik Nayak 

Since there are multiple atoms which print refs ('%(refname)',
'%(symref)', '%(push)', '%(upstream)'), it makes sense to have a common
ground for parsing them. This would allow us to share implementations of
the atom modifiers between these atoms.

Introduce refname_atom_parser_internal() to act as a common parsing
function for ref printing atoms. This would eventually be used to
introduce refname_atom_parser() and symref_atom_parser() and also be
internally used in remote_ref_atom_parser().

Helped-by: Jeff King 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/ref-filter.c b/ref-filter.c
index 405a0e5..1a18c7d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -31,6 +31,11 @@ struct if_then_else {
condition_satisfied : 1;
 };
 
+struct refname_atom {
+   enum { R_NORMAL, R_SHORT, R_STRIP } option;
+   unsigned int strip;
+};
+
 /*
  * An atom is a valid field atom listed below, possibly prefixed with
  * a "*" to denote deref_tag().
@@ -63,6 +68,7 @@ static struct used_atom {
enum { O_FULL, O_LENGTH, O_SHORT } option;
unsigned int length;
} objectname;
+   struct refname_atom refname;
} u;
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
@@ -76,6 +82,21 @@ static void color_atom_parser(struct used_atom *atom, const 
char *color_value)
die(_("unrecognized color: %%(color:%s)"), color_value);
 }
 
+static void refname_atom_parser_internal(struct refname_atom *atom,
+const char *arg, const char *name)
+{
+   if (!arg)
+   atom->option = R_NORMAL;
+   else if (!strcmp(arg, "short"))
+   atom->option = R_SHORT;
+   else if (skip_prefix(arg, "strip=", )) {
+   atom->option = R_STRIP;
+   if (strtoul_ui(arg, 10, >strip) || atom->strip <= 0)
+   die(_("positive value expected refname:strip=%s"), arg);
+   } else
+   die(_("unrecognized %%(%s) argument: %s"), name, arg);
+}
+
 static void remote_ref_atom_parser(struct used_atom *atom, const char *arg)
 {
struct string_list params = STRING_LIST_INIT_DUP;
-- 
2.10.2

[PATCH v8 11/19] ref-filter: introduce refname_atom_parser()

[Karthik Nayak]

From: Karthik Nayak 

Using refname_atom_parser_internal(), introduce refname_atom_parser()
which will parse the %(symref) and %(refname) atoms. Store the parsed
information into the 'used_atom' structure based on the modifiers used
along with the atoms.

Now the '%(symref)' atom supports the ':strip' atom modifier. Update the
Documentation and tests to reflect this.

Helped-by: Jeff King 
Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt |  5 +++
 ref-filter.c   | 73 +-
 t/t6300-for-each-ref.sh|  9 +
 3 files changed, 54 insertions(+), 33 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index e1d250e..8224f37 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -167,6 +167,11 @@ if::
the value between the %(if:...) and %(then) atoms with the
given string.
 
+symref::
+   The ref which the given symbolic ref refers to. If not a
+   symbolic ref, nothing is printed. Respects the `:short` and
+   `:strip` options in the same way as `refname` above.
+
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
 be used to specify the value in the header field.
diff --git a/ref-filter.c b/ref-filter.c
index 1a18c7d..9f5522d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -177,6 +177,11 @@ static void objectname_atom_parser(struct used_atom *atom, 
const char *arg)
die(_("unrecognized %%(objectname) argument: %s"), arg);
 }
 
+static void refname_atom_parser(struct used_atom *atom, const char *arg)
+{
+   return refname_atom_parser_internal(>u.refname, arg, atom->name);
+}
+
 static align_type parse_align_position(const char *s)
 {
if (!strcmp(s, "right"))
@@ -247,7 +252,7 @@ static struct {
cmp_type cmp_type;
void (*parser)(struct used_atom *atom, const char *arg);
 } valid_atom[] = {
-   { "refname" },
+   { "refname" , FIELD_STR, refname_atom_parser },
{ "objecttype" },
{ "objectsize", FIELD_ULONG },
{ "objectname", FIELD_STR, objectname_atom_parser },
@@ -276,7 +281,7 @@ static struct {
{ "contents", FIELD_STR, contents_atom_parser },
{ "upstream", FIELD_STR, remote_ref_atom_parser },
{ "push", FIELD_STR, remote_ref_atom_parser },
-   { "symref" },
+   { "symref", FIELD_STR, refname_atom_parser },
{ "flag" },
{ "HEAD" },
{ "color", FIELD_STR, color_atom_parser },
@@ -1062,21 +1067,16 @@ static inline char *copy_advance(char *dst, const char 
*src)
return dst;
 }
 
-static const char *strip_ref_components(const char *refname, const char 
*nr_arg)
+static const char *strip_ref_components(const char *refname, unsigned int len)
 {
-   char *end;
-   long nr = strtol(nr_arg, , 10);
-   long remaining = nr;
+   long remaining = len;
const char *start = refname;
 
-   if (nr < 1 || *end != '\0')
-   die(_(":strip= requires a positive integer argument"));
-
while (remaining) {
switch (*start++) {
case '\0':
-   die(_("ref '%s' does not have %ld components to 
:strip"),
-   refname, nr);
+   die(_("ref '%s' does not have %ud components to 
:strip"),
+   refname, len);
case '/':
remaining--;
break;
@@ -1085,6 +1085,16 @@ static const char *strip_ref_components(const char 
*refname, const char *nr_arg)
return start;
 }
 
+static const char *show_ref(struct refname_atom *atom, const char *refname)
+{
+   if (atom->option == R_SHORT)
+   return shorten_unambiguous_ref(refname, warn_ambiguous_refs);
+   else if (atom->option == R_STRIP)
+   return strip_ref_components(refname, atom->strip);
+   else
+   return refname;
+}
+
 static void fill_remote_ref_details(struct used_atom *atom, const char 
*refname,
struct branch *branch, const char **s)
 {
@@ -1157,6 +1167,21 @@ char *get_head_description(void)
return strbuf_detach(, NULL);
 }
 
+static const char *get_symref(struct used_atom *atom, struct ref_array_item 
*ref)
+{
+   if (!ref->symref)
+   return "";
+   else
+   return show_ref(>u.refname, ref->symref);
+}
+
+static const char *get_refname(struct used_atom *atom, struct ref_array_item 
*ref)
+{
+   if (ref->kind & FILTER_REFS_DETACHED_HEAD)
+   return get_head_description();
+   return show_ref(>u.refname, ref->refname);
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -1185,7 +1210,6 @@ static void populate_value(struct 

[PATCH v8 15/19] ref-filter: add an 'rstrip=' option to atoms which deal with refnames

[Karthik Nayak]

Complimenting the existing 'lstrip=' option, add an 'rstrip='
option which strips `` slash-separated path components from the end
of the refname (e.g., `%(refname:rstrip=2)` turns `refs/tags/foo` into
`refs`).

Signed-off-by: Karthik Nayak 
---
 Documentation/git-for-each-ref.txt | 40 -
 ref-filter.c   | 41 --
 t/t6300-for-each-ref.sh| 24 ++
 3 files changed, 85 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index ad9b243..c72baeb 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -92,10 +92,12 @@ refname::
The name of the ref (the part after $GIT_DIR/).
For a non-ambiguous short name of the ref append `:short`.
The option core.warnAmbiguousRefs is used to select the strict
-   abbreviation mode. If `lstrip=` is appended, strips ``
-   slash-separated path components from the front of the refname
-   (e.g., `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo`.
-   if `` is a negative number, then only `` path components
+   abbreviation mode. If `lstrip=` or `rstrip=` option can
+   be appended to strip `` slash-separated path components
+   from or end of the refname respectively (e.g.,
+   `%(refname:lstrip=2)` turns `refs/tags/foo` into `foo` and
+   `%(refname:rstrip=2)` turns `refs/tags/foo` into `refs`).  if
+   `` is a negative number, then only `` path components
are left behind.  If a displayed ref has fewer components than
``, the command aborts with an error.
 
@@ -114,22 +116,23 @@ objectname::
 
 upstream::
The name of a local ref which can be considered ``upstream''
-   from the displayed ref. Respects `:short` and `:lstrip` in the
-   same way as `refname` above.  Additionally respects `:track`
-   to show "[ahead N, behind M]" and `:trackshort` to show the
-   terse version: ">" (ahead), "<" (behind), "<>" (ahead and
-   behind), or "=" (in sync). `:track` also prints "[gone]"
-   whenever unknown upstream ref is encountered. Append
-   `:track,nobracket` to show tracking information without
-   brackets (i.e "ahead N, behind M").  Has no effect if the ref
-   does not have tracking information associated with it.  All
-   the options apart from `nobracket` are mutually exclusive, but
-   if used together the last option is selected.
+   from the displayed ref. Respects `:short`, `:lstrip` and
+   `:rstrip` in the same way as `refname` above.  Additionally
+   respects `:track` to show "[ahead N, behind M]" and
+   `:trackshort` to show the terse version: ">" (ahead), "<"
+   (behind), "<>" (ahead and behind), or "=" (in sync). `:track`
+   also prints "[gone]" whenever unknown upstream ref is
+   encountered. Append `:track,nobracket` to show tracking
+   information without brackets (i.e "ahead N, behind M").  Has
+   no effect if the ref does not have tracking information
+   associated with it.  All the options apart from `nobracket`
+   are mutually exclusive, but if used together the last option
+   is selected.
 
 push::
The name of a local ref which represents the `@{push}`
location for the displayed ref. Respects `:short`, `:lstrip`,
-   `:track`, and `:trackshort` options as `upstream`
+   `:rstrip`, `:track`, and `:trackshort` options as `upstream`
does. Produces an empty string if no `@{push}` ref is
configured.
 
@@ -171,8 +174,9 @@ if::
 
 symref::
The ref which the given symbolic ref refers to. If not a
-   symbolic ref, nothing is printed. Respects the `:short` and
-   `:lstrip` options in the same way as `refname` above.
+   symbolic ref, nothing is printed. Respects the `:short`,
+   `:lstrip` and `:rstrip` options in the same way as `refname`
+   above.
 
 In addition to the above, for commit and tag objects, the header
 field names (`tree`, `parent`, `object`, `type`, and `tag`) can
diff --git a/ref-filter.c b/ref-filter.c
index deb2b29..9fce5bb 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -32,8 +32,8 @@ struct if_then_else {
 };
 
 struct refname_atom {
-   enum { R_NORMAL, R_SHORT, R_LSTRIP } option;
-   int lstrip;
+   enum { R_NORMAL, R_SHORT, R_LSTRIP, R_RSTRIP } option;
+   int lstrip, rstrip;
 };
 
 /*
@@ -94,6 +94,10 @@ static void refname_atom_parser_internal(struct refname_atom 
*atom,
atom->option = R_LSTRIP;
if (strtol_i(arg, 10, >lstrip))
die(_("Integer value expected refname:lstrip=%s"), arg);
+   } else if (skip_prefix(arg, "rstrip=", )) {
+   atom->option = R_RSTRIP;
+   if (strtol_i(arg, 10, >rstrip))
+   die(_("Integer value