Re: [PATCH] run-command: async_exit no longer needs to be public

[Ramsay Jones]

On 23/09/16 20:26, Junio C Hamano wrote:
> Lars Schneider  writes:
> 
>>> I do not offhand know if the topic is otherwise ready as-is, or
>>> needs further work.  When you need to reroll, you'd also need to
>>> fetch from the result of the above from me first and then start your
>>> work from it, though, if we go that route.
>>
>> Sounds good to me!
> 
> OK, here is what I queued, then.

This looks good to me. Thanks!

ATB,
Ramsay Jones

[PATCH 1/3 v3] submodules: make submodule-prefix option an envvar

[Brandon Williams]

Add a submodule-prefix enviorment variable
'GIT_INTERNAL_SUBMODULE_PREFIX' which can be used by commands which have
--recurse-submodule options.

Signed-off-by: Brandon Williams 
---
 cache.h   | 1 +
 environment.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/cache.h b/cache.h
index 3556326..ae88a35 100644
--- a/cache.h
+++ b/cache.h
@@ -408,6 +408,7 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define GIT_PREFIX_ENVIRONMENT "GIT_PREFIX"
+#define GIT_SUBMODULE_PREFIX_ENVIRONMENT "GIT_INTERNAL_SUBMODULE_PREFIX"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
diff --git a/environment.c b/environment.c
index ca72464..7380815 100644
--- a/environment.c
+++ b/environment.c
@@ -120,6 +120,7 @@ const char * const local_repo_env[] = {
NO_REPLACE_OBJECTS_ENVIRONMENT,
GIT_REPLACE_REF_BASE_ENVIRONMENT,
GIT_PREFIX_ENVIRONMENT,
+   GIT_SUBMODULE_PREFIX_ENVIRONMENT,
GIT_SHALLOW_FILE_ENVIRONMENT,
GIT_COMMON_DIR_ENVIRONMENT,
NULL
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/3 v3] ls-files: optionally recurse into submodules

[Brandon Williams]

Allow ls-files to recognize submodules in order to retrieve a list of
files from a repository's submodules.  This is done by forking off a
process to recursively call ls-files on all submodules. Use environment
variable `GIT_INTERNAL_SUBMODULE_PREFIX` to pass a path to the submodule
which it can use to prepend to output or pathspec matching logic.

Signed-off-by: Brandon Williams 
---
 Documentation/git-ls-files.txt |  7 ++-
 builtin/ls-files.c | 63 ++
 t/t3007-ls-files-recurse-submodules.sh | 99 ++
 3 files changed, 168 insertions(+), 1 deletion(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

diff --git a/Documentation/git-ls-files.txt b/Documentation/git-ls-files.txt
index 0d933ac..446209e 100644
--- a/Documentation/git-ls-files.txt
+++ b/Documentation/git-ls-files.txt
@@ -18,7 +18,8 @@ SYNOPSIS
[--exclude-per-directory=]
[--exclude-standard]
[--error-unmatch] [--with-tree=]
-   [--full-name] [--abbrev] [--] [...]
+   [--full-name] [--recurse-submodules]
+   [--abbrev] [--] [...]
 
 DESCRIPTION
 ---
@@ -137,6 +138,10 @@ a space) at the start of each line:
option forces paths to be output relative to the project
top directory.
 
+--recurse-submodules::
+   Recursively calls ls-files on each submodule in the repository.
+   Currently there is only support for the --cached mode.
+
 --abbrev[=]::
Instead of showing the full 40-byte hexadecimal object
lines, show only a partial prefix.
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 00ea91a..54ab765 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -14,6 +14,7 @@
 #include "resolve-undo.h"
 #include "string-list.h"
 #include "pathspec.h"
+#include "run-command.h"
 
 static int abbrev;
 static int show_deleted;
@@ -28,6 +29,8 @@ static int show_valid_bit;
 static int line_terminator = '\n';
 static int debug_mode;
 static int show_eol;
+static int recurse_submodules;
+static const char *submodule_prefix;
 
 static const char *prefix;
 static int max_prefix_len;
@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, 
const char *path)
 static void write_name(const char *name)
 {
/*
+* NEEDSWORK: To make this thread-safe, full_name would have to be owned
+* by the caller.
+*
+* full_name get reused across output lines to minimize the allocation
+* churn.
+*/
+   static struct strbuf full_name = STRBUF_INIT;
+   if (submodule_prefix && *submodule_prefix) {
+   strbuf_reset(_name);
+   strbuf_addstr(_name, submodule_prefix);
+   strbuf_addstr(_name, name);
+   name = full_name.buf;
+   }
+
+   /*
 * With "--full-name", prefix_len=0; this caller needs to pass
 * an empty string in that case (a NULL is good for "").
 */
@@ -152,6 +170,27 @@ static void show_killed_files(struct dir_struct *dir)
}
 }
 
+/**
+ * Recursively call ls-files on a submodule
+ */
+static void show_gitlink(const struct cache_entry *ce)
+{
+   struct child_process cp = CHILD_PROCESS_INIT;
+   int status;
+
+   argv_array_push(, "ls-files");
+   argv_array_push(, "--recurse-submodules");
+   argv_array_pushf(_array, "%s=%s%s/",
+GIT_SUBMODULE_PREFIX_ENVIRONMENT,
+submodule_prefix ? submodule_prefix : "",
+ce->name);
+   cp.git_cmd = 1;
+   cp.dir = ce->name;
+   status = run_command();
+   if (status)
+   exit(status);
+}
+
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
int len = max_prefix_len;
@@ -163,6 +202,10 @@ static void show_ce_entry(const char *tag, const struct 
cache_entry *ce)
len, ps_matched,
S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
return;
+   if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+   show_gitlink(ce);
+   return;
+   }
 
if (tag && *tag && show_valid_bit &&
(ce->ce_flags & CE_VALID)) {
@@ -468,6 +511,8 @@ int cmd_ls_files(int argc, const char **argv, const char 
*cmd_prefix)
{ OPTION_SET_INT, 0, "full-name", _len, NULL,
N_("make the output relative to the project top 
directory"),
PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
+   OPT_BOOL(0, "recurse-submodules", _submodules,
+   N_("recurse through submodules")),
OPT_BOOL(0, "error-unmatch", _unmatch,
N_("if any  is not in the index, treat this as an 
error")),
OPT_STRING(0, "with-tree", _tree, N_("tree-ish"),
@@ -519,6 +564,24 @@ int 

[PATCH 3/3 v3] ls-files: add pathspec matching for submodules

[Brandon Williams]

Pathspecs can be a bit tricky when trying to apply them to submodules.
The main challenge is that the pathspecs will be with respect to the
superproject and not with respect to paths in the submodule.  The
approach this patch takes is to pass in the identical pathspec from the
superproject to the submodule in addition to the submodule-prefix, which
is the path from the root of the superproject to the submodule, and then
we can compare an entry in the submodule prepended with the
submodule-prefix to the pathspec in order to determine if there is a
match.

This patch also permits the pathspec logic to perform a prefix match against
submodules since a pathspec could refer to a file inside of a submodule.
Due to limitations in the wildmatch logic, a prefix match is only done
literally.  If any wildcard character is encountered we'll simply punt
and produce a false positive match.  More accurate matching will be done
once inside the submodule.  This is due to the superproject not knowing
what files could exist in the submodule.

Signed-off-by: Brandon Williams 
---
 builtin/ls-files.c | 134 -
 dir.c  |  46 ++-
 dir.h  |   4 +
 t/t3007-ls-files-recurse-submodules.sh | 126 +--
 4 files changed, 248 insertions(+), 62 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 54ab765..8ecffd1 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -177,6 +177,7 @@ static void show_gitlink(const struct cache_entry *ce)
 {
struct child_process cp = CHILD_PROCESS_INIT;
int status;
+   int i;
 
argv_array_push(, "ls-files");
argv_array_push(, "--recurse-submodules");
@@ -184,6 +185,29 @@ static void show_gitlink(const struct cache_entry *ce)
 GIT_SUBMODULE_PREFIX_ENVIRONMENT,
 submodule_prefix ? submodule_prefix : "",
 ce->name);
+   /* add options */
+   if (show_eol)
+   argv_array_push(, "--eol");
+   if (show_valid_bit)
+   argv_array_push(, "-v");
+   if (show_stage)
+   argv_array_push(, "--stage");
+   if (show_cached)
+   argv_array_push(, "--cached");
+   if (debug_mode)
+   argv_array_push(, "--debug");
+   if (line_terminator == '\0')
+   argv_array_push(, "-z");
+
+   /*
+* Pass in the original pathspec args.  The submodule will be
+* responsible for prepending the 'submodule_prefix' prior to comparing
+* against the pathspec for matches.
+*/
+   argv_array_push(, "--");
+   for (i = 0; i < pathspec.nr; i++)
+   argv_array_push(, pathspec.items[i].original);
+
cp.git_cmd = 1;
cp.dir = ce->name;
status = run_command();
@@ -193,57 +217,62 @@ static void show_gitlink(const struct cache_entry *ce)
 
 static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 {
+   struct strbuf name = STRBUF_INIT;
int len = max_prefix_len;
+   if (submodule_prefix)
+   strbuf_addstr(, submodule_prefix);
+   strbuf_addstr(, ce->name);
 
if (len >= ce_namelen(ce))
die("git ls-files: internal error - cache entry not superset of 
prefix");
 
-   if (!match_pathspec(, ce->name, ce_namelen(ce),
-   len, ps_matched,
-   S_ISDIR(ce->ce_mode) || S_ISGITLINK(ce->ce_mode)))
-   return;
-   if (recurse_submodules && S_ISGITLINK(ce->ce_mode)) {
+   if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+   submodule_path_match(, name.buf, ps_matched)) {
show_gitlink(ce);
-   return;
-   }
+   } else if (match_pathspec(, name.buf, name.len,
+ len, ps_matched,
+ S_ISDIR(ce->ce_mode) ||
+ S_ISGITLINK(ce->ce_mode))) {
+   if (tag && *tag && show_valid_bit &&
+   (ce->ce_flags & CE_VALID)) {
+   static char alttag[4];
+   memcpy(alttag, tag, 3);
+   if (isalpha(tag[0]))
+   alttag[0] = tolower(tag[0]);
+   else if (tag[0] == '?')
+   alttag[0] = '!';
+   else {
+   alttag[0] = 'v';
+   alttag[1] = tag[0];
+   alttag[2] = ' ';
+   alttag[3] = 0;
+   }
+   tag = alttag;
+   }
 
-   if (tag && *tag && show_valid_bit &&
-   (ce->ce_flags & CE_VALID)) {
-   static char alttag[4];
-   memcpy(alttag, tag, 3);
-   if 

[PATCH 0/3] recursive support for ls-files

[Brandon Williams]

After looking at the feedback I rerolled a few things, in particular the
--submodule_prefix option that existed to give a submodule context about where
it had been invoked from.  People didn't seem to like the idea of exposing this
to the users (yet anyways) so I removed it as an option and instead have it
being passed to a child process via an environment variable
GIT_INTERNAL_SUBMODULE_PREFIX.  This way we don't have to support anything to
external users at the moment.

Also fixed a bug (and added a test) for the -z options as pointed out by Jeff
King.

Brandon Williams (3):
  submodules: make submodule-prefix option an envvar
  ls-files: optionally recurse into submodules
  ls-files: add pathspec matching for submodules

 Documentation/git-ls-files.txt |   7 +-
 builtin/ls-files.c | 173 ---
 cache.h|   1 +
 dir.c  |  46 +++-
 dir.h  |   4 +
 environment.c  |   1 +
 t/t3007-ls-files-recurse-submodules.sh | 209 +
 7 files changed, 398 insertions(+), 43 deletions(-)
 create mode 100755 t/t3007-ls-files-recurse-submodules.sh

-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 1/2] ls-files: optionally recurse into submodules

[Brandon Williams]

On Wed, Sep 21, 2016 at 11:20 PM, Jeff King  wrote:
>> +/**
>> + * Recursively call ls-files on a submodule
>> + */
>> +static void show_gitlink(const struct cache_entry *ce)
>> +{
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + int status;
>> +
>> + argv_array_push(, "ls-files");
>> + argv_array_push(, "--recurse-submodules");
>> + argv_array_pushf(, "--submodule-prefix=%s%s/",
>> +  submodule_prefix ? submodule_prefix : "",
>> +  ce->name);
>> + cp.git_cmd = 1;
>> + cp.dir = ce->name;
>> + status = run_command();
>> + if (status)
>> + exit(status);
>> +}
>
> This doesn't propagate the parent argv at all. So if I run:
>
>   git ls-files -z --recurse-submodules
>
> then the paths are all NUL-terminated in the parent, but
> newline-terminated in the submodules. Oops.

Yep definitely missed that.  I can fix that.  I think the main reason
for not blindly
copying the argv array is that there may be some things we don't want to pass
to the child. While not in the context of ls-files, I was working on
recursive grep
earlier and with that you can pass a rev to grep.  You can't blindly
copy that because
 the rev is meaningless to the the child and may produce broken
output.  Instead we
would need to pass the actual rev of what the parent has checked out
in that particular
rev.  I haven't thought it completely through yet but it did
discourage me from blindly
copying the args across.

-Brandon

What's cooking in git.git (Sep 2016, #07; Fri, 23)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

A bunch of topics have graduated to 'next', including a few that
were so far marked as "needs review" or "will hold", as I think
giving them a greater visibility and guinea pigs would be the most
efficient way to get feedback from the real world ;-) Some of them
may be "Meh" topic, which might be why they weren't getting any
feedback so far, but at least this way we'd know if there are
breakages in them (in which case we can just revert and discard
them).

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* jk/clone-recursive-progress (2016-09-22) 1 commit
  (merged to 'next' on 2016-09-22 at 8310c42)
 + clone: pass --progress decision to recursive submodules

 "git clone --recurse-submodules" lost the progress eye-candy in
 recent update, which has been corrected.

 Will merge to 'master'.


* jk/doc-cvs-update (2016-09-22) 3 commits
  (merged to 'next' on 2016-09-22 at c0f949f)
 + docs/cvs-migration: mention cvsimport caveats
 + docs/cvs-migration: update link to cvsps homepage
 + docs/cvsimport: prefer cvs-fast-export to parsecvs

 Documentation around tools to import from CVS was fairly outdated.

 Will merge to 'master'.


* jk/verify-packfile-gently (2016-09-22) 1 commit
 - verify_packfile: check pack validity before accessing data

 A low-level function verify_packfile() was meant to show errors
 detected without dying itself, but under some conditions it didn't
 and died instead, which has been fixed.

 Will merge to 'next'.


* jt/fetch-pack-in-vain-count-with-stateless (2016-09-23) 1 commit
 - fetch-pack: do not reset in_vain on non-novel acks

 When "git fetch" tries to find where the history it has diverged
 from what the other side has, it has a mechanism to avoid digging
 too deep into irrelevant side branches.  This however did not work
 well over the "smart-http" transport due to a design bug, which has
 been fixed.

 Will merge to 'next'.


* rs/checkout-init-macro (2016-09-22) 1 commit
  (merged to 'next' on 2016-09-22 at 6513755)
 + introduce CHECKOUT_INIT

 Code cleanup.

 Will merge to 'master'.


* ik/gitweb-force-highlight (2016-09-23) 2 commits
 - gitweb: use highlight's shebang detection
 - gitweb: remove unused paarmeter from guess_file_syntax()

 "gitweb" can spawn "highlight" to show blob contents with
 (programming) language-specific syntax highlighting, but only
 when the language is known.  "highlight" can however be told
 to make the guess itself by giving it "--force" option, which
 has been enabled.

 Waiting for the discussion to conclude.
 cf. <2a4c3efb-2145-b699-c980-3079f165a...@gmail.com>


* jk/ident-ai-canonname-could-be-null (2016-09-23) 1 commit
 - ident: handle NULL ai_canonname

 In the codepath that comes up with the hostname to be used in an
 e-mail when the user didn't tell us, we looked at ai_canonname
 field in struct addrinfo without making sure it is not NULL first.

 Will merge to 'next'.

--
[Stalled]

* jc/bundle (2016-03-03) 6 commits
 - index-pack: --clone-bundle option
 - Merge branch 'jc/index-pack' into jc/bundle
 - bundle v3: the beginning
 - bundle: keep a copy of bundle file name in the in-core bundle header
 - bundle: plug resource leak
 - bundle doc: 'verify' is not about verifying the bundle

 The beginning of "split bundle", which could be one of the
 ingredients to allow "git clone" traffic off of the core server
 network to CDN.

 While I think it would make it easier for people to experiment and
 build on if the topic is merged to 'next', I am at the same time a
 bit reluctant to merge an unproven new topic that introduces a new
 file format, which we may end up having to support til the end of
 time.  It is likely that to support a "prime clone from CDN", it
 would need a lot more than just "these are the heads and the pack
 data is over there", so this may not be sufficient.

 Will discard.


* jc/attr (2016-05-25) 18 commits
 - attr: support quoting pathname patterns in C style
 - attr: expose validity check for attribute names
 - attr: add counted string version of git_attr()
 - attr: add counted string version of git_check_attr()
 - attr: retire git_check_attrs() API
 - attr: convert git_check_attrs() callers to use the new API
 - attr: convert git_all_attrs() to use "struct git_attr_check"
 - attr: (re)introduce git_check_attr() and struct git_attr_check
 - attr: rename function and struct related to checking attributes
 - attr.c: plug small leak in parse_attr_line()
 - attr.c: tighten constness around "git_attr" structure
 - attr.c: simplify 

Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

>  I think a separate commit for this is better than combining back to
>  2/3 so we can explain the problem properly (without making 2/3 commit
>  message even longer)
>
>  Not sure if you want to s/contains/contain/ in 2/3 by yourself or I
>  should resend the whole series. Let me know.

I think this 4/3 is not quite enough to fix the damage to the code
caused by 2/3.

Given that

 - set_git_dir_init() is is the only one that sets original_git_dir,

 - create_default_files() is the only one that uses
   original_git_dir, and

 - init_db() is the only one that calls set_git_dir_init() and
   create_default_files()

after 4/3 is applied, we should be able to remove the global
variable 2/3 introduced, make init_db() receive that information as
the return value of set_git_dir_init(), and pass that as a parameter
to create_default_files().

Re: .gitignore does not ignore Makefile

[Jakub Narębski]

W dniu 22.09.2016 o 20:26, Junio C Hamano napisał:
> Kevin Daudt  writes:
> 
>> Often people advise tricks like `git update-index --assume-unchanges
>> `, but this does not work as expected. It's merely a promise to
>> git that this file does not change (and hence, git will not check if
>> this file has changed when doing git status), but command that try to
>> change this file will abort saying that the file has changed.
> 
> It actually is even worse.  As the user promised Git that the 
> will not be modified and will be kept the same as the version in the
> index, Git reserves the right to _overwrite_ it with the version in
> the index anytime when it is convenient to do so, removing whatever
> local change the user had despite the promise to Git.  The "abort
> saying that the file has changed" is merely various codepaths in the
> current implementation trying to be extra nice.
 
There is a trick that works almost as 'ignore changes' for tracked
files, namely `git update-index --skip-worktree `.  From the
documentation:

  Skip-worktree bit
  ~

  Skip-worktree bit can be defined in one (long) sentence: When
  reading an entry, if it is marked as skip-worktree, then Git
  pretends its working directory version is up to date and read
  the index version instead.

  [...] Writing is not affected by this bit, content safety is still
  first priority. [...]

It works quite well; the only problem is that `git stash` would
not stash away your changes, and you would need to unmark such
file before saving a stash.


With --assume-unchanged used for ignoring changes to tracked files,
you can quite easily lose your work because you are lying to Git.


Note also that in Git classic "ignored" implies unimportant.
-- 
Jakub Narębski

Re: Limitiations of git rebase --preserve-merges --interactive

[Stefan Beller]

On Fri, Sep 23, 2016 at 2:13 PM, Johannes Schindelin
 wrote:
> Hi Stefan,
>
> On Fri, 23 Sep 2016, Stefan Haller wrote:
>
>> Stefan Beller  wrote:
>>
>> > On Thu, Sep 22, 2016 at 12:48 PM, Kevin Daudt  wrote:
>> > > On Thu, Sep 22, 2016 at 07:33:11PM +, Anatoly Borodin wrote:
>> > >> Hi Stefan,
>> > >>
>> > >> this section was added to the manual in the commit
>> > >> cddb42d2c58a9de9b2b5ef68817778e7afaace3e by "Jonathan Nieder"
>> > >>  6 years ago. Maybe he remembers better?
>> > >>
>> > >
>> > > Just to make it clear, this section explicitly talks about 'bugs' with
>> > > preserve-merges and interactive rebase.  Without the --preserve-merges
>> > > option, those operations works as expected.
>> > >
>> > > The reason, as that section explains, is that it's not possible to store
>> > > the merge structure in the flat todo list. I assume this means git
>> > > internally remembers where the merge commit was, and then restores it
>> > > while rebasing.
>> > >
>> > > Changing the order, or dropping commits might then give unexpected
>> > > results.
>> > >
>> >
>> > The commit message may help as well:
>> >
>> > rebase -i -p: document shortcomings
>> >
>> > The rebase --preserve-merges facility presents a list of commits
>> > in its instruction sheet and uses a separate table to keep
>> > track of their parents.  Unfortunately, in practice this means
>> > that with -p after most attempts to rearrange patches, some
>> > commits have the "wrong" parent and the resulting history is
>> > rarely what the caller expected.
>> >
>> > Yes, it would be nice to fix that.  But first, add a warning to the
>> > manual to help the uninitiated understand what is going on.
>>
>> Thanks, but all of this still talks about the issues in very generic
>> terms ("most attempts to rearrange patches"). I'm interested in more
>> details as to exactly what kind of attempts do or don't work. In
>> particular, I'm interested in fixup/squash commands (without reordering
>> anything else), or dropping (non-merge) commits.
>>
>> I could of course experiment with these and try to find out myself, but
>> I was hoping someone would just know the answer off the top of their
>> head, saving me some time.
>
> The fundamental problem here is the underlying design of bolting on the
> "recreate a merge" functionality onto the "pick" command.
>
> That is, if you try to rebase non-linear commit history, it will still
> generate a linear list of "pick " lines, as if it were
> linear, except that it will include the merge commits, too.

Which on a more fundamental design level would be ok.
(C.f. your shell history is a linear list of git commands, but it
deals just fine
with non linear DAGSs)

>
> It then will try to guess what you want to do by recording which commit
> was rewritten as which commit. And when it encounters a "pick" with a
> merge commit, it will try to merge the *rewritten* commit.

Instead of guessing we'd need to differentiate between "pick" and "pickmerge",
whereas the later describes creating commits with more than one parent (i.e.
the prior pick line).

I could imagine the "pickmerge" to list all additional parents (The
first parent being
the previously picked commit) via symbolic naming:

pick 1234affe implement foo
pickmerge 3456feed origin/js/new-feature-1 # Merge origin/js/new-feature-1
pick 45678ead implement feature-2

The "pickmerge" would have first the merge tips, and then the old
subject line after
a # character.

>
> In other words, the design does not allow for changing the tip of any
> merged branch. Not reordering, not dropping.

I see how the current design is problematic as there is no argument
possible that
allows the user to correct the wrong guess.

>
> And I do not think that there is a way to fix that design. That is why I
> came up with the Git garden shears (see the link I sent elsewhere in this
> thread).

I'll look into that.

Thanks,
Stefan

Re: [PATCH v3 2/2] gitweb: use highlight's shebang detection

[Jakub Narębski]

W dniu 23.09.2016 o 11:08, Ian Kelling napisał:

> The "highlight" binary can, in some cases, determine the language type
> by the means of file contents, for example the shebang in the first line
> for some scripting languages.  Make use of this autodetection for files
> which syntax is not known by gitweb.  In that case, pass the blob
> contents to "highlight --force"; the parameter is needed to make it
> always generate HTML output (which includes HTML-escaping).

Right.

> 
> Although we now run highlight on files which do not end up highlighted,
> performance is virtually unaffected because when we call highlight, we
> also call sanitize() instead of esc_html(), which is significantly
> slower. 

This paragraph is a bit unclear, for example it is not obvious what
"..., which is significantly slower" refers to: sanitize() or esc_html().

I think it would be better to write:

  Although we now run highlight on files which do not end up highlighted,
  performance is virtually unaffected because when we call highlight, it
  is used for escaping HTML.  In the case that highlight is used, gitweb
  calls sanitize() instead of esc_html(), and the latter is significantly
  slower (it does more, being roughly a superset of sanitize()).

>After curling blob view of unhighlighted large and small text
> files of perl code and license text 100 times each on a local
> Apache/2.4.23 (Debian) instance, it's logs indicate +-1% difference in
> request time for all file types.

Also, "curling" is not the word I would like to see. I would say:

  Simple benchmark comparing performance of 'blob' view of files without
  syntax highlighting in gitweb before and after this change indicates
  ±1% difference in request time for all file types.  Benchmark was
  performed on local instance on Debian, using Apache/2.4.23 web server
  and CGI/PSGI/FCGI/mod_perl.

  ^^--- select one

Or something like that; I'm not sure how detailed this should be.
But it is nice to have such benchmark in the commit message.

Anyway I think that adding yet another configuration toggle for selecting
whether to use "highlight" syntax autodetection or not would be just an
unnecessary complication.

Note that the performance loss might be quite higher on MS Windows, with
its higher cost of fork.  But then they probably do not configure
server-side highligher anyway.

> 
> Document the feature and improve syntax highlight documentation, add
> test to ensure gitweb doesn't crash when language detection is used.

Good.

> 
> Signed-off-by: Ian Kelling 
> ---
>  Documentation/gitweb.conf.txt  | 21 ++---
>  gitweb/gitweb.perl | 10 +-
>  t/t9500-gitweb-standalone-no-errors.sh |  8 
>  3 files changed, 27 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> index a79e350..e632089 100644
> --- a/Documentation/gitweb.conf.txt
> +++ b/Documentation/gitweb.conf.txt
> @@ -246,13 +246,20 @@ $highlight_bin::

We should probably say what does it mean to be "highlight"[1] compatible,
but it is outside of scope for this patch, and I think also out of scope
of this series.

>   Note that 'highlight' feature must be set for gitweb to actually
>   use syntax highlighting.
>  +
> -*NOTE*: if you want to add support for new file type (supported by
> -"highlight" but not used by gitweb), you need to modify `%highlight_ext`
> -or `%highlight_basename`, depending on whether you detect type of file
> -based on extension (for example "sh") or on its basename (for example
> -"Makefile").  The keys of these hashes are extension and basename,
> -respectively, and value for given key is name of syntax to be passed via
> -`--syntax ` to highlighter.
> +*NOTE*: for a file to be highlighted, its syntax type must be detected
> +and that syntax must be supported by "highlight".  The default syntax
> +detection is minimal, and there are many supported syntax types with no
> +detection by default.  There are three options for adding syntax
> +detection.  The first and second priority are `%highlight_basename` and
> +`%highlight_ext`, which detect based on basename (the full filename, for
> +example "Makefile") and extension (for example "sh").  The keys of these
> +hashes are the basename and extension, respectively, and the value for a
> +given key is the name of the syntax to be passed via `--syntax `
> +to "highlight".  The last priority is the "highlight" configuration of
> +`Shebang` regular expressions to detect the language based on the first
> +line in the file, (for example, matching the line "#!/bin/bash").  See
> +the highlight documentation and the default config at
> +/etc/highlight/filetypes.conf for more details.

All right. I guess /etc/highlight/filetypes.conf is the standard location?

>  +
>  For example if repositories you are hosting use "phtml" extension for
>  PHP files, and you want to 

Re: Limitiations of git rebase --preserve-merges --interactive

[Johannes Schindelin]

Hi Stefan,

On Fri, 23 Sep 2016, Stefan Haller wrote:

> Stefan Beller  wrote:
> 
> > On Thu, Sep 22, 2016 at 12:48 PM, Kevin Daudt  wrote:
> > > On Thu, Sep 22, 2016 at 07:33:11PM +, Anatoly Borodin wrote:
> > >> Hi Stefan,
> > >>
> > >> this section was added to the manual in the commit
> > >> cddb42d2c58a9de9b2b5ef68817778e7afaace3e by "Jonathan Nieder"
> > >>  6 years ago. Maybe he remembers better?
> > >>
> > >
> > > Just to make it clear, this section explicitly talks about 'bugs' with
> > > preserve-merges and interactive rebase.  Without the --preserve-merges
> > > option, those operations works as expected.
> > >
> > > The reason, as that section explains, is that it's not possible to store
> > > the merge structure in the flat todo list. I assume this means git
> > > internally remembers where the merge commit was, and then restores it
> > > while rebasing.
> > >
> > > Changing the order, or dropping commits might then give unexpected
> > > results.
> > >
> > 
> > The commit message may help as well:
> > 
> > rebase -i -p: document shortcomings
> > 
> > The rebase --preserve-merges facility presents a list of commits
> > in its instruction sheet and uses a separate table to keep
> > track of their parents.  Unfortunately, in practice this means
> > that with -p after most attempts to rearrange patches, some
> > commits have the "wrong" parent and the resulting history is
> > rarely what the caller expected.
> > 
> > Yes, it would be nice to fix that.  But first, add a warning to the
> > manual to help the uninitiated understand what is going on.
> 
> Thanks, but all of this still talks about the issues in very generic
> terms ("most attempts to rearrange patches"). I'm interested in more
> details as to exactly what kind of attempts do or don't work. In
> particular, I'm interested in fixup/squash commands (without reordering
> anything else), or dropping (non-merge) commits.
> 
> I could of course experiment with these and try to find out myself, but
> I was hoping someone would just know the answer off the top of their
> head, saving me some time.

The fundamental problem here is the underlying design of bolting on the
"recreate a merge" functionality onto the "pick" command.

That is, if you try to rebase non-linear commit history, it will still
generate a linear list of "pick " lines, as if it were
linear, except that it will include the merge commits, too.

It then will try to guess what you want to do by recording which commit
was rewritten as which commit. And when it encounters a "pick" with a
merge commit, it will try to merge the *rewritten* commit.

In other words, the design does not allow for changing the tip of any
merged branch. Not reordering, not dropping.

And I do not think that there is a way to fix that design. That is why I
came up with the Git garden shears (see the link I sent elsewhere in this
thread).

Ciao,
Johannes

Re: Limitiations of git rebase --preserve-merges --interactive

[Johannes Schindelin]

Hi,

On Thu, 22 Sep 2016, Stefan Beller wrote:

> On Thu, Sep 22, 2016 at 2:01 PM, Anatoly Borodin
>  wrote:
> > Hi Stefan,
> >
> > I've also done some archaeology and found that the original version of
> > the merge preserving code was written by Johannes Schindelin
> > , see e.g.
> 
> I think it would be helpful if you'd cc those folks involved, not just
> the mailing list.

Indeed. It is quite tedious to re-Cc: people. Anatoly, please do not force
me to put in that work in the future.

Also: I mentioned recently that I am not happy with the original design,
either, and that I came up with a new design that I intend to port to the
rebase--helper, once it is included in an official Git version:

https://public-inbox.org/git/alpine.DEB.2.20.1609111027330.129229@virtualbox/

Ciao,
Johannes

Re: What's cooking in git.git (Sep 2016, #06; Wed, 21)

[Johannes Schindelin]

Hi,

On Wed, 21 Sep 2016, Junio C Hamano wrote:

> * rt/rebase-i-broken-insn-advise (2016-09-07) 1 commit
>  - rebase -i: improve advice on bad instruction lines
> 
>  When "git rebase -i" is given a broken instruction, it told the
>  user to fix it with "--edit-todo", but didn't say what the step
>  after that was (i.e. "--continue").
> 
>  Will hold.
>  Dscho's "rebase -i" hopefully will become available in 'pu', by
>  which time an equivalent of this fix would be ported to C.  This is
>  queued merely as a reminder.

Porting the fix was surprisingly easy:

-- snipsnap --
[PATCH] fixup! rebase -i: check for missing commits in the rebase--helper

---
 sequencer.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 8f27524..386d16e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2567,9 +2567,10 @@ int check_todo_list(void)
 
if (raise_error)
fprintf(stderr,
-   _("You can fix this with 'git rebase --edit-todo'.\n"
-   "Or you can abort the rebase with 'git rebase"
-   " --abort'.\n"));
+   _("You can fix this with 'git rebase --edit-todo' "
+ "and then run 'git rebase --continue'.\n"
+ "Or you can abort the rebase with 'git rebase"
+ " --abort'.\n"));
 
return res;
 }
-- 
2.10.0.windows.1.10.g803177d

Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules

[Brandon Williams]

On Fri, Sep 23, 2016 at 12:20 PM, Junio C Hamano  wrote:
> There is an interesting observation around this code.  Note that it
> is just something to keep in mind, even though I think we are in no
> position to solve this within the scope of this series, or in fact I
> am not sure if there is anything to "fix".
>
> The expectation here is that the leading part of pathspec elements
> contain path components above and outside the current working tree,
> e.g. in a superproject with a submodule at "sub/", the end-user may
> have said from the top of the superproject
>
> A saving grace is that "s*b/file" in this case is what the end-user
> is giving us, not something we internally generated.  So we can
> simply blame the end user, saying "what --recurse-submodules does is
> to (conceptually) flatten the indices of submodules into the index
> of the superproject and show the entries that match your pathspec.
> Because you gave us 's*b/file', which does match 's*b/oob/file',
> that is what you get."
>
> ;-)

Yeah I've been thinking a bit about that as well.  To me, it is
incredibly silly to
have a wildcard character in a filename (its unfortunate that its
allowed).  We can
easily do as you suggest and simply blame the user and if they do have wildcard
characters in their filenames they would just need to force the
pathspec code to
do checks literally (using the appropriate pathspec magic).  This
would just limit their
ability to use actual wildcards in their pathspecs, ie they have to
pick wildcards in their
filenames or the ability to do wildmatching.

-Brandon

Re: [PATCH v3 1/2] gitweb: remove unused function parameter

[Junio C Hamano]

Jakub Narębski  writes:

> I think it would be better to be more descriptive, and say:
>
>   Subject: [PATCH v3 1/2] gitweb: remove unused parameter from 
> guess_file_syntax()
> Acked-by: Jakub Narębski 

Thanks.

Re: [PATCH v3 1/2] gitweb: remove unused function parameter

[Jakub Narębski]

W dniu 23.09.2016 o 11:08, Ian Kelling napisał:
>
> Subject: [PATCH v3 1/2] gitweb: remove unused function parameter

I think it would be better to be more descriptive, and say:

  Subject: [PATCH v3 1/2] gitweb: remove unused parameter from 
guess_file_syntax()

But that might be too long...

>
> Signed-off-by: Ian Kelling 

With, or without this change, it's nice.

Acked-by: Jakub Narębski 

> ---
>  gitweb/gitweb.perl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 33d701d..6cb4280 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3913,7 +3913,7 @@ sub blob_contenttype {
>  # guess file syntax for syntax highlighting; return undef if no highlighting
>  # the name of syntax can (in the future) depend on syntax highlighter used
>  sub guess_file_syntax {
> - my ($highlight, $mimetype, $file_name) = @_;
> + my ($highlight, $file_name) = @_;
>   return undef unless ($highlight && defined $file_name);
>   my $basename = basename($file_name, '.in');
>   return $highlight_basename{$basename}
> @@ -7062,7 +7062,7 @@ sub git_blob {
>   $have_blame &&= ($mimetype =~ m!^text/!);
>  
>   my $highlight = gitweb_check_feature('highlight');
> - my $syntax = guess_file_syntax($highlight, $mimetype, $file_name);
> + my $syntax = guess_file_syntax($highlight, $file_name);
>   $fd = run_highlighter($fd, $highlight, $syntax)
>   if $syntax;
>  
> 

Re: [PATCH v2] fetch-pack: do not reset in_vain on non-novel acks

[Junio C Hamano]

Jonathan Tan  writes:

> I tried looking at creating a helper function to reduce both the size
> and the nesting level of the loop, but it seems to me that a helper
> function can't be extracted so easily because the logic is quite
> intertwined with the rest of the function. For example, the "if
> (args->stateless_rpc..." block uses 6 variables from the outer scope:
> args, ack, commit, result_sha1, req_buf, and state_len (and in_vain, but
> this can be the return value of the function). Expanding it wider would
> allow us to make some of those 6 local, but also introduce new ones from
> the outer scope.

Yup, I suspected that much when I wrote the message you are
responding to, but was sort-of hoping that you might come up with a
more clever way to restructure the code.  It is OK to leave it
as-is, and let others try making it cleaner ;-).

Thanks.

>
>  fetch-pack.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fetch-pack.c b/fetch-pack.c
> index 85e77af..413937e 100644
> --- a/fetch-pack.c
> +++ b/fetch-pack.c
> @@ -428,10 +428,17 @@ static int find_common(struct fetch_pack_args *args,
>   const char *hex = 
> sha1_to_hex(result_sha1);
>   packet_buf_write(_buf, 
> "have %s\n", hex);
>   state_len = req_buf.len;
> - }
> + /*
> +  * Reset in_vain because an ack
> +  * for this commit has not been
> +  * seen.
> +  */
> + in_vain = 0;
> + } else if (!args->stateless_rpc
> +|| ack != ACK_common)
> + in_vain = 0;
>   mark_common(commit, 0, 1);
>   retval = 0;
> - in_vain = 0;
>   got_continue = 1;
>   if (ack == ACK_ready) {
>   clear_prio_queue(_list);

Re: [PATCH] run-command: async_exit no longer needs to be public

[Junio C Hamano]

Lars Schneider  writes:

>> I do not offhand know if the topic is otherwise ready as-is, or
>> needs further work.  When you need to reroll, you'd also need to
>> fetch from the result of the above from me first and then start your
>> work from it, though, if we go that route.
>
> Sounds good to me!

OK, here is what I queued, then.

-- >8 --
From: Lars Schneider 
Date: Tue, 20 Sep 2016 21:02:39 +0200
Subject: [PATCH] run-command: move check_pipe() from write_or_die to
 run_command

Move check_pipe() to run_command and make it public. This is necessary
to call the function from pkt-line in a subsequent patch.

While at it, make async_exit() static to run_command.c as it is no
longer used from outside.

Signed-off-by: Lars Schneider 
Signed-off-by: Ramsay Jones 
Signed-off-by: Junio C Hamano 
---
 run-command.c  | 17 +++--
 run-command.h  |  2 +-
 write_or_die.c | 13 -
 3 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/run-command.c b/run-command.c
index 5a4dbb6..3269362 100644
--- a/run-command.c
+++ b/run-command.c
@@ -634,7 +634,7 @@ int in_async(void)
return !pthread_equal(main_thread, pthread_self());
 }
 
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
 {
pthread_exit((void *)(intptr_t)code);
 }
@@ -684,13 +684,26 @@ int in_async(void)
return process_is_async;
 }
 
-void NORETURN async_exit(int code)
+static void NORETURN async_exit(int code)
 {
exit(code);
 }
 
 #endif
 
+void check_pipe(int err)
+{
+   if (err == EPIPE) {
+   if (in_async())
+   async_exit(141);
+
+   signal(SIGPIPE, SIG_DFL);
+   raise(SIGPIPE);
+   /* Should never happen, but just in case... */
+   exit(141);
+   }
+}
+
 int start_async(struct async *async)
 {
int need_in, need_out;
diff --git a/run-command.h b/run-command.h
index 5066649..cf29a31 100644
--- a/run-command.h
+++ b/run-command.h
@@ -139,7 +139,7 @@ struct async {
 int start_async(struct async *async);
 int finish_async(struct async *async);
 int in_async(void);
-void NORETURN async_exit(int code);
+void check_pipe(int err);
 
 /**
  * This callback should initialize the child process and preload the
diff --git a/write_or_die.c b/write_or_die.c
index 0734432..eab8c8d 100644
--- a/write_or_die.c
+++ b/write_or_die.c
@@ -1,19 +1,6 @@
 #include "cache.h"
 #include "run-command.h"
 
-static void check_pipe(int err)
-{
-   if (err == EPIPE) {
-   if (in_async())
-   async_exit(141);
-
-   signal(SIGPIPE, SIG_DFL);
-   raise(SIGPIPE);
-   /* Should never happen, but just in case... */
-   exit(141);
-   }
-}
-
 /*
  * Some cases use stdio, but want to flush after the write
  * to get error handling (and to get better interactive
-- 
2.10.0-530-g67247c9

Re: Limitiations of git rebase --preserve-merges --interactive

[Johannes Sixt]

Am 23.09.2016 um 17:50 schrieb Stefan Haller:

And I don't see any tests that do rebase -p -i and actually do something
interesting with the -i part. So my original question still remains. :-)


-i -p came first. -p without -i was bolted on later.

-- Hannes

Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules

[Junio C Hamano]

Brandon Williams  writes:

>  static void show_ce_entry(const char *tag, const struct cache_entry *ce)
>  {
> + struct strbuf name = STRBUF_INIT;
>   int len = max_prefix_len;
> + if (submodule_prefix)
> + strbuf_addstr(, submodule_prefix);
> + strbuf_addstr(, ce->name);
> ...  
> + } else if (match_pathspec(, name.buf, name.len,
> +   len, ps_matched,
> +   S_ISDIR(ce->ce_mode) ||
> +   S_ISGITLINK(ce->ce_mode))) {

There is an interesting observation around this code.  Note that it
is just something to keep in mind, even though I think we are in no
position to solve this within the scope of this series, or in fact I
am not sure if there is anything to "fix".

The expectation here is that the leading part of pathspec elements
contain path components above and outside the current working tree,
e.g. in a superproject with a submodule at "sub/", the end-user may
have said from the top of the superproject

git ls-files --recurse-submodules -- sub/file

and the recursing "ls-files" is spawned as

git -C sub ls-files -- sub/file

relaying the pathspec literally.

This does not correctly work if the path to the submodule has
wildcard in it.  Imagine that the submodule were at "s*b/".  The
recursing invocation would look like:

git -C "s*b" ls-files -- "s*b/file"

Further imagine that the index in the submodule at "s*b" has two
paths in it, i.e.

file
oob/file

The prefix is prepended to them, to turn them into

s*b/file
s*b/oob/file

and I suspect that the pathspec element "s*b/file" would match both
of them.

The pathspec machinery has a provision to prevent a similar gotcha
happening for the "prefix" we internally use.  In a sample
repository created like so:

$ git init
$ mkdir -p 's*b/oob' sib
$ >sib/file
$ cd 's*b'
$ >file
$ >oob/file
$ git add .
$ git ls-files -- file

the "ls-files" in the last step gets 's*b/' as the "prefix", and the
pathspec is formed by concatenating "file" to it, but in a special
way.  The part that come from the "prefix" is marked not to honor
any wildcard in it, so 's*b/' even though it has an asterisk, it is
forced to match literally, giving only 's*b/file'.

A saving grace is that "s*b/file" in this case is what the end-user
is giving us, not something we internally generated.  So we can
simply blame the end user, saying "what --recurse-submodules does is
to (conceptually) flatten the indices of submodules into the index
of the superproject and show the entries that match your pathspec.
Because you gave us 's*b/file', which does match 's*b/oob/file',
that is what you get."

;-)

Re: [PATCH] run-command: async_exit no longer needs to be public

[Lars Schneider]

> On 23 Sep 2016, at 19:13, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>>> If you need to re-roll your 'ls/filter-process' branch, could you please
>>> squash this into the relevant commit c42a4cbc ("run-command: move 
>>> check_pipe()
>>> from write_or_die to run_command", 20-09-2016).
>>> 
>>> [Note that commit 9658846c ("write_or_die: handle EPIPE in async threads",
>>> 24-02-2016) introduced async_exit() specifically for use in the 
>>> implementation
>>> of check_pipe(). Now that you have moved check_pipe() into run-command.c,
>>> it no longer needs to be public.]
>> 
>> Hi Ramsay,
>> 
>> thanks for noticing this. I actually hope that I don't need another re-roll 
>> :-)
>> If I don't re-roll. Should I make a patch with this cleanup or do you
>> take care of it?
> 
> I can just squash the the patch you are responding to into c42a4cbc,
> with an additional paragraph "While at it, retire async_exit() as a
> public function as it no longer is called outside run-command API
> implementation", or something like that.
> 
> I do not offhand know if the topic is otherwise ready as-is, or
> needs further work.  When you need to reroll, you'd also need to
> fetch from the result of the above from me first and then start your
> work from it, though, if we go that route.

Sounds good to me!

Thank you, Junio!

Re: [PATCH 2/2 v2] ls-files: add pathspec matching for submodules

[Junio C Hamano]

Brandon Williams  writes:

> - /* Find common prefix for all pathspec's */
> - max_prefix = common_prefix();
> + /*
> +  * Find common prefix for all pathspec's
> +  * This is used as a performance optimization which unfortunately cannot
> +  * be done when recursing into submodules
> +  */
> + if (recurse_submodules)
> + max_prefix = NULL;
> + else
> + max_prefix = common_prefix();
>   max_prefix_len = max_prefix ? strlen(max_prefix) : 0;

This is OK for now, but for a future enhancement, I think we could
do better than this.  In a superproject with a submodule at "sub/",
the current implementation of the common_prefix() helper would yield
"sub/a/" when given "sub/a/x" and "sub/a/y" (a pathspec with two
elements), which we want to avoid.

But somebody should be able to notice, before "sub/a/" is given to
max_prefix here, that "sub/" is the leaf level in our repository and
reduce the max_prefix to it.  dir.c::common_prefix_len() might be 
a place we could do so, but I didn't think about the ramifications
of doing so for other callers of common_prefix() or when we are not
recursing into submodules.  Doing it in the caller here, i.e.

max_prefix = common_prefix();
if (recurse_submodules)
max_prefix = chomp_at_submodule_boundary(max_prefix);

is certainly safer.

If the superproject has submodules at "a/b/{sub1,sub2,...}", this
matters more.  We do want to notice that we won't have to scan
outside "a/b/" of the index given "a/b/sub1" and "a/b/sub2" as a
pathspec.

The common_prefix_len() function also looks beyond symbolic links,
which is another thing that we may want to think about.  In a
repository with a symbolic link "link" pointing somewhere else, when
you give "link/a/x" and "link/a/y" (a pathspec with two elements),
we would get "link/a/" as a common prefix, but we won't find
anything underneath "link" in our index.  In such a case, leaving
the common prefix to "link/a/" _might_ allow us to notice that no
pathspec elements can ever match, so not noticing that the common
prefix points beyond a symbolic link might be a feature.  I dunno.

[PATCH v2] fetch-pack: do not reset in_vain on non-novel acks

[Jonathan Tan]

The MAX_IN_VAIN mechanism was introduced in commit f061e5f ("fetch-pack:
give up after getting too many "ack continue"", 2006-05-24) to stop ref
negotiation if a number of consecutive "have"s have been sent with no
corresponding new acks. This is to stop the client from digging too deep
in an irrelevant side branch in vain without ever finding a common
ancestor. A use case (as described in that commit) is the scenario in
which the local repository has more roots than the remote repository.

However, during a negotiation in which stateless RPCs are used,
MAX_IN_VAIN will (almost) never trigger (in the more-roots scenario
above and others) because in each new request, the client has to inform
the server of objects it already has and knows the server has (to remind
the server of the state), which the server then acks.

Make fetch-pack only consider, as new acks for the purpose of
MAX_IN_VAIN, acks for objects for which the client has never received an
ack before in this session.

Signed-off-by: Jonathan Tan 
---

Thanks for your comments - I really appreciate them.

Update from original:
o removed redundant text from commit message and comment in patch
o mentioned stopping the client from digging too deep in the commit
  message

I tried looking at creating a helper function to reduce both the size
and the nesting level of the loop, but it seems to me that a helper
function can't be extracted so easily because the logic is quite
intertwined with the rest of the function. For example, the "if
(args->stateless_rpc..." block uses 6 variables from the outer scope:
args, ack, commit, result_sha1, req_buf, and state_len (and in_vain, but
this can be the return value of the function). Expanding it wider would
allow us to make some of those 6 local, but also introduce new ones from
the outer scope.

 fetch-pack.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fetch-pack.c b/fetch-pack.c
index 85e77af..413937e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -428,10 +428,17 @@ static int find_common(struct fetch_pack_args *args,
const char *hex = 
sha1_to_hex(result_sha1);
packet_buf_write(_buf, 
"have %s\n", hex);
state_len = req_buf.len;
-   }
+   /*
+* Reset in_vain because an ack
+* for this commit has not been
+* seen.
+*/
+   in_vain = 0;
+   } else if (!args->stateless_rpc
+  || ack != ACK_common)
+   in_vain = 0;
mark_common(commit, 0, 1);
retval = 0;
-   in_vain = 0;
got_continue = 1;
if (ack == ACK_ready) {
clear_prio_queue(_list);
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] run-command: async_exit no longer needs to be public

[Junio C Hamano]

Lars Schneider  writes:

>> If you need to re-roll your 'ls/filter-process' branch, could you please
>> squash this into the relevant commit c42a4cbc ("run-command: move 
>> check_pipe()
>> from write_or_die to run_command", 20-09-2016).
>> 
>> [Note that commit 9658846c ("write_or_die: handle EPIPE in async threads",
>> 24-02-2016) introduced async_exit() specifically for use in the 
>> implementation
>> of check_pipe(). Now that you have moved check_pipe() into run-command.c,
>> it no longer needs to be public.]
>
> Hi Ramsay,
>
> thanks for noticing this. I actually hope that I don't need another re-roll 
> :-)
> If I don't re-roll. Should I make a patch with this cleanup or do you
> take care of it?

I can just squash the the patch you are responding to into c42a4cbc,
with an additional paragraph "While at it, retire async_exit() as a
public function as it no longer is called outside run-command API
implementation", or something like that.

I do not offhand know if the topic is otherwise ready as-is, or
needs further work.  When you need to reroll, you'd also need to
fetch from the result of the above from me first and then start your
work from it, though, if we go that route.

Re: [RFC PATCH] revision: new rev%n shorthand for rev^n..rev

[Junio C Hamano]

Vegard Nossum  writes:

> I use rev^..rev daily, and I'm surely not the only one. To save typing
> (or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
> we can make rev% a shorthand for that.

No, we cannot.

'%' is not reserved as a special character that is forbidden in
reference names, and for somebody who has a branch whose name is
'master%', such a change will suddenly make 'master%' mean something
completely different, breaking existing users' repositories.

This is why existing rev^@ and rev^! both use the "^" as the first
character that introduces the "magic" semantics; "^" cannot be a
part of a refname.  Also sequences that begin with "^{" and "@{" are
reserved as escape hatches to allow us extend the revision syntax in
the future ("^{" works on history, while "@{" bases its working on
the reflog data).

As "rev^$n" is "nth parent", it may be a possibility to use "rev^-$n"
as a short-hand for "^rev^$n rev".

Re: [PATCH 1/2] ls-files: adding support for submodules

[Stefan Beller]

On Fri, Sep 23, 2016 at 9:16 AM, Brandon Williams  wrote:
>> Yeah, a positive "I support this" flag would at least let us correctly
>> flag errors, which is the best we can do. That won't work for
>> non-builtins, but perhaps it is good enough in practice.
>>
>> -Peff
>
>
> So it sounds like we agree that this prefix option should be pushed to
> the top level.
> The question is have we come to a consensus on what we should be
> calling the option?

The option itself is very similar to -C, which changes the directory to the
given argument before executing the git command.
e.g. in git:

git -C builtin ls-files
add.c
...

So for the submodule case we'd want that plus keeping around that prefix,
which makes me wonder if we could just store the argument of -C into a global
and use that when --keep-prefix is given, so you'd do a

git -C path/to/sub --keep-prefix ls-files
path/to/sub/file1
...

maybe --[keep|use]-[path|prefix] ?

You could of course go with a fully independent option, but how
would that work together with -C ?
(first change the dir and then change again while remembering the prefix?
or the other way round?)

> Leave it as submodule-prefix or do we need to come up with a different name?
>
> -Brandon

Re: [PATCH 1/2] ls-files: adding support for submodules

[Brandon Williams]

> Yeah, a positive "I support this" flag would at least let us correctly
> flag errors, which is the best we can do. That won't work for
> non-builtins, but perhaps it is good enough in practice.
>
> -Peff


So it sounds like we agree that this prefix option should be pushed to
the top level.
The question is have we come to a consensus on what we should be
calling the option?
Leave it as submodule-prefix or do we need to come up with a different name?

-Brandon

Re: Limitiations of git rebase --preserve-merges --interactive

[Junio C Hamano]

li...@haller-berlin.de (Stefan Haller) writes:

> Thanks, this is interesting; I'm having trouble understanding the tests
> though. Some of them use rebase -p -i, but I don't understand why they
> use -i, or why that even works in a test (i.e. why it doesn't open an
> editor).

Upon starting up, tests dot-source t/test-lib.sh file and it
unsets most of GIT_* environment variables to obtain a stable
testing environment that is not affected by things that testers
may have in their environment.

There is EDITOR=: in t/test-lib.sh, which was added in 2006 before
GIT_EDITOR was invented.  That is the one in effect for git
subcommands that usually interacts with editors during the test,
unless specific tests further override it with test_set_editor
helper.

Re: [PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

>  I think a separate commit for this is better than combining back to
>  2/3 so we can explain the problem properly (without making 2/3 commit
>  message even longer)
>
>  Not sure if you want to s/contains/contain/ in 2/3 by yourself or I
>  should resend the whole series. Let me know.

OK, I just amended it before applying this on top.

> + flags |= INIT_DB_EXIST_OK;
> + return init_db(git_dir, real_git_dir, template_dir, flags);

I do not think of anything better, but EXIST_OK does not sound
grammatical.  "REINIT" is not quite it--we are merely allowing
the function to re-init if there already is a repository.  And
OK_TO_REINIT is a bit too long.  Let's take the patch as-is for
now.

Thanks.

Re: [PATCH 4/6] tag: add format specifier to gpg_verify_tag

[Santiago Torres]

On Thu, Sep 22, 2016 at 01:58:06PM -0700, Junio C Hamano wrote:
> santi...@nyu.edu writes:
> 
> > Calling functions for gpg_verify_tag() may desire to print relevant
> > information about the header for further verification. Add an optional
> > format argument to print any desired information after GPG verification.
> 
> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index dbf271f..94ed8a2 100644
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > @@ -106,7 +106,7 @@ static int delete_tag(const char *name, const char *ref,
> >  static int verify_tag(const char *name, const char *ref,
> > const unsigned char *sha1)
> >  {
> > -   return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
> > +   return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
> >  }
> >  
> >  static int do_sign(struct strbuf *buffer)
> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> > index 99f8148..7a1121b 100644
> > --- a/builtin/verify-tag.c
> > +++ b/builtin/verify-tag.c
> > @@ -51,8 +51,10 @@ int cmd_verify_tag(int argc, const char **argv, const 
> > char *prefix)
> > const char *name = argv[i++];
> > if (get_sha1(name, sha1))
> > had_error = !!error("tag '%s' not found.", name);
> > -   else if (gpg_verify_tag(sha1, name, flags))
> > -   had_error = 1;
> > +   else {
> > +   if (verify_and_format_tag(sha1, name, NULL, flags))
> > +   had_error = 1;
> > +   }
> 
> Revert the unnecessary reformatting here.
> 
> > @@ -56,6 +57,15 @@ int gpg_verify_tag(const unsigned char *sha1, const char 
> > *name_to_report,
> > ret = run_gpg_verify(buf, size, flags);
> >  
> > free(buf);
> > +
> > +   if (fmt_pretty) {
> > +   struct ref_array_item *ref_item;
> > +   ref_item = new_ref_item(name_to_report, sha1, 0);
> > +   ref_item->kind = FILTER_REFS_TAGS;
> > +   show_ref_item(ref_item, fmt_pretty, 0);
> > +   free_ref_item(ref_item);
> > +   }
> 
> I haven't seen 5/6 and 6/6, but if this is the only user of the 3/6,
> it would be much better to have a single function to format a ref
> exported from ref-filter.[ch] so that this one can say
> 
>   if (fmt_pretty)
>   format_ref(name_to_report, sha1, FILTER_REFS_TAGS);
> 
> or something like that, instead of doing three that will always be
> used together in quick succession in the above pattern.

Oh, this sounds like a better alternative. This would be instead of 0003
right? 

Thanks,
-Santiago.


signature.asc
Description: PGP signature

Re: [PATCH 5/6] builtin/verify-tag: Add --format to verify-tag

[Santiago Torres]

On Thu, Sep 22, 2016 at 02:16:21PM -0700, Junio C Hamano wrote:
> santi...@nyu.edu writes:
> 
> > From: Santiago Torres 
> >
> > Callers of verify-tag may want to cross-check the tagname from refs/tags
> > with the tagname from the tag object header upon GPG verification. This
> > is to avoid tag refs that point to an incorrect object.
> >
> > Add a --format parameter to git verify-tag to print the formatted tag
> > object header in addition to or instead of the --verbose or --raw GPG
> > verification output.
> >
> > Signed-off-by: Santiago Torres 
> > ---
> >  builtin/verify-tag.c | 13 +++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> > index 7a1121b..319d469 100644
> > --- a/builtin/verify-tag.c
> > +++ b/builtin/verify-tag.c
> > @@ -12,12 +12,15 @@
> >  #include 
> >  #include "parse-options.h"
> >  #include "gpg-interface.h"
> > +#include "ref-filter.h"
> >  
> >  static const char * const verify_tag_usage[] = {
> > -   N_("git verify-tag [-v | --verbose] ..."),
> > +   N_("git verify-tag [-v | --verbose] [--format=] 
> > ..."),
> > NULL
> >  };
> >  
> > +char *fmt_pretty;
> 
> Does this have to be extern?  I do not think so; prepend "static "
> in front of it.
> 
> > while (i < argc) {
> > unsigned char sha1[20];
> > const char *name = argv[i++];
> > if (get_sha1(name, sha1))
> > had_error = !!error("tag '%s' not found.", name);
> > else {
> > -   if (verify_and_format_tag(sha1, name, NULL, flags))
> > +   if (verify_and_format_tag(sha1, name, fmt_pretty, 
> > flags))
> 
> OK.  The callchain from here is
> 
> verify_and_format_tag()
> -> run_gpg_verify()
>   -> print_signature_buffer()
> 
> so not cramming QUIET into the flags parameter that is already
> passed is cumbersome.  As I said in my earlier review, it would make
> more sense to have the conditional NOT in print_signature_buffer()
> but in its caller, but it still is OK to add GPG_VERIFY_QUIET bit
> to the flag, which you would check in run_gpg_verify() to decide not
> to call print_signature_buffer().
> 

Yeah, in retrospect, this sounds like a more reasonable approach than
doing it on gpg-nterface. I'll keep the QUIET bit then.


signature.asc
Description: PGP signature

Re: [PATCH 6/6] builtin/tag: add --format argument for tag -v

[Santiago Torres]

> OK, you said something about for_each_ref() in an earlier commit,
> but what you meant was this one, which takes each_tag_name_fn.

Oh yeah, sorry for the confusion.

> 
> The function for_each_tag_name(), the type each_tag_name_fn, and the
> function of that type verify_tag(), are ALL file-scope static in
> this single file, builtin/tag.c.  It seems to me that it is not
> necessary to make the format string global at all.

Oh, ok. I was thinking that this was preferred over changing the
signature of those functions. (I drew my conclusion from log.c). I'll
take this other road then.
> 
> ... 
> 
> There are minor implementation and design issues I spotted, but
> overall I think the feature the series attempts to add may be a good
> thing to have.
> 

Thanks for the review! I'll re-roll shortly.
-Santiago. 


> Thanks.


signature.asc
Description: PGP signature

[PATCH v2 4/3] init: combine set_git_dir_init() and init_db() into one

[Nguyễn Thái Ngọc Duy]

Commit "init: do not set core.worktree more often than necessary" adds a
subtle dependency between set_git_dir_init() and init_db(). The former
_must_ be called before init_db() so that original_git_dir can be set
properly. If something else, like enter_repo() or setup_git_directory(),
is used instead, the trick in that commit breaks down.

To eliminate the possibility that init_db() in future may be called
without set_git_dir_init(), init_db() now calls that function internally
(and does not allow anybody else to use it).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 I think a separate commit for this is better than combining back to
 2/3 so we can explain the problem properly (without making 2/3 commit
 message even longer)

 Not sure if you want to s/contains/contain/ in 2/3 by yourself or I
 should resend the whole series. Let me know.

 builtin/clone.c   | 15 +++
 builtin/init-db.c | 18 +++---
 cache.h   |  5 +++--
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 6616392..29b1832 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -928,23 +928,22 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
set_git_work_tree(work_tree);
}
 
-   junk_git_dir = git_dir;
+   junk_git_dir = real_git_dir ? real_git_dir : git_dir;
if (safe_create_leading_directories_const(git_dir) < 0)
die(_("could not create leading directories of '%s'"), git_dir);
 
-   set_git_dir_init(git_dir, real_git_dir, 0);
-   if (real_git_dir) {
-   git_dir = real_git_dir;
-   junk_git_dir = real_git_dir;
-   }
-
if (0 <= option_verbosity) {
if (option_bare)
fprintf(stderr, _("Cloning into bare repository 
'%s'...\n"), dir);
else
fprintf(stderr, _("Cloning into '%s'...\n"), dir);
}
-   init_db(option_template, INIT_DB_QUIET);
+
+   init_db(git_dir, real_git_dir, option_template, INIT_DB_QUIET);
+
+   if (real_git_dir)
+   git_dir = real_git_dir;
+
write_config(_config);
 
git_config(git_default_config, NULL);
diff --git a/builtin/init-db.c b/builtin/init-db.c
index d70fc45..ee7942f 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -312,8 +312,9 @@ static void create_object_directory(void)
strbuf_release();
 }
 
-int set_git_dir_init(const char *git_dir, const char *real_git_dir,
-int exist_ok)
+static int set_git_dir_init(const char *git_dir,
+   const char *real_git_dir,
+   int exist_ok)
 {
original_git_dir = xstrdup(real_path(git_dir));
 
@@ -362,10 +363,14 @@ static void separate_git_dir(const char *git_dir)
write_file(git_link, "gitdir: %s", git_dir);
 }
 
-int init_db(const char *template_dir, unsigned int flags)
+int init_db(const char *git_dir, const char *real_git_dir,
+   const char *template_dir, unsigned int flags)
 {
int reinit;
-   const char *git_dir = get_git_dir();
+
+   set_git_dir_init(git_dir, real_git_dir, flags & INIT_DB_EXIST_OK);
+
+   git_dir = get_git_dir();
 
if (git_link)
separate_git_dir(git_dir);
@@ -585,7 +590,6 @@ int cmd_init_db(int argc, const char **argv, const char 
*prefix)
set_git_work_tree(work_tree);
}
 
-   set_git_dir_init(git_dir, real_git_dir, 1);
-
-   return init_db(template_dir, flags);
+   flags |= INIT_DB_EXIST_OK;
+   return init_db(git_dir, real_git_dir, template_dir, flags);
 }
diff --git a/cache.h b/cache.h
index b2d77f3..7fc875f 100644
--- a/cache.h
+++ b/cache.h
@@ -525,9 +525,10 @@ extern void verify_non_filename(const char *prefix, const 
char *name);
 extern int path_inside_repo(const char *prefix, const char *path);
 
 #define INIT_DB_QUIET 0x0001
+#define INIT_DB_EXIST_OK 0x0002
 
-extern int set_git_dir_init(const char *git_dir, const char *real_git_dir, 
int);
-extern int init_db(const char *template_dir, unsigned int flags);
+extern int init_db(const char *git_dir, const char *real_git_dir,
+  const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
 extern int daemonize(void);
-- 
2.8.2.524.g6ff3d78

[RFC PATCH] revision: new rev%n shorthand for rev^n..rev

[Vegard Nossum]

I use rev^..rev daily, and I'm surely not the only one. To save typing
(or copy-pasting, if the rev is long -- like a full SHA-1 or branch name)
we can make rev% a shorthand for that.

The existing syntax rev^! seems like it should do the same, but it
doesn't really do the right thing for merge commits (it gives only the
merge itself).

As a natural generalisation, we also accept rev%n where n excludes the
nth parent of rev. It _may_ be more useful to define rev%n for an m-way
merge as:

 rev
 ^rev^1
 ^rev^[... except n]
 ^rev^m

so that you can see only the commits that arrived via the nth parent,
but this might be questionable/unintuitive in case any of the parents
that share commits (as you would get fewer commits than expected).

Signed-off-by: Vegard Nossum 
---
 Documentation/revisions.txt | 14 +
 builtin/rev-parse.c | 38 ++
 revision.c  | 50 +
 3 files changed, 102 insertions(+)

diff --git Documentation/revisions.txt Documentation/revisions.txt
index 4bed5b1..ab2dc2c 100644
--- Documentation/revisions.txt
+++ Documentation/revisions.txt
@@ -281,6 +281,14 @@ is a shorthand for 'HEAD..origin' and asks "What did the 
origin do since
 I forked from them?"  Note that '..' would mean 'HEAD..HEAD' which is an
 empty range that is both reachable and unreachable from HEAD.
 
+Parent Exclusion Notation
+~
+The '%{}', Parent Exclusion Notation::
+Shorthand for '{caret}..', with '' = 1 if not
+given. This is typically useful for merge commits where you
+can just pass '%' to get all the commits in the branch
+that was merged in merge commit ''.
+
 Other {caret} Parent Shorthand Notations
 ~
 Two other shorthands exist, particularly useful for merge commits,
@@ -316,6 +324,10 @@ Revision Range Summary
 but exclude those that are reachable from both.  When
either  or  is omitted, it defaults to `HEAD`.
 
+'%{}', e.g. 'HEAD%, HEAD%2'::
+   Equivalent to '{caret}..', with '' = 1 if not
+   given.
+
 '{caret}@', e.g. 'HEAD{caret}@'::
   A suffix '{caret}' followed by an at sign is the same as listing
   all parents of '' (meaning, include anything reachable from
@@ -339,6 +351,8 @@ spelt out:
CI J F C
B..C   = ^B CC
B...C  = B ^F C  G H D E B C
+   B% = B^..B
+ = B ^B^1  E I J F B
C^@= C^1
  = F   I J F
B^@= B^1 B^2 B^3
diff --git builtin/rev-parse.c builtin/rev-parse.c
index 76cf05e..f081b81 100644
--- builtin/rev-parse.c
+++ builtin/rev-parse.c
@@ -292,6 +292,42 @@ static int try_difference(const char *arg)
return 0;
 }
 
+static int try_branch(const char *arg)
+{
+   char *percent;
+   unsigned char sha1[20];
+   unsigned char end[20];
+
+   /*
+* %{} is shorthand for ^.., with  = 1 if
+* not given. This is typically used for merge commits where you
+* can just pass % and it will show you all the commits in
+* the branch that was merged (for octopus merges,  is the nth
+* branch).
+*/
+
+   if (!(percent = strstr(arg, "%")))
+   return 0;
+
+   *percent = '^';
+   if (!get_sha1_committish(arg, sha1)) {
+   *percent = '%';
+   return 0;
+   }
+
+   *percent = '\0';
+   if (!get_sha1_committish(arg, end)) {
+   *percent = '%';
+   return 0;
+   }
+
+   show_rev(NORMAL, end, arg);
+   *percent = '^';
+   show_rev(REVERSED, sha1, arg);
+   *percent = '%';
+   return 1;
+}
+
 static int try_parent_shorthands(const char *arg)
 {
char *dotdot;
@@ -839,6 +875,8 @@ int cmd_rev_parse(int argc, const char **argv, const char 
*prefix)
/* Not a flag argument */
if (try_difference(arg))
continue;
+   if (try_branch(arg))
+   continue;
if (try_parent_shorthands(arg))
continue;
name = arg;
diff --git revision.c revision.c
index 969b3d1..e20b618 100644
--- revision.c
+++ revision.c
@@ -1519,6 +1519,56 @@ int handle_revision_arg(const char *arg_, struct 
rev_info *revs, int flags, unsi
}
*dotdot = '.';
}
+
+   /*
+* %{} is shorthand for ^.., with  = 1 if
+* not given. This is typically used for merge commits where you
+* can just pass % and it will show you all the commits in
+* the branch that was merged (for octopus merges,  is the nth
+* branch).
+*/
+   dotdot = strstr(arg, "%");
+   if (dotdot) {
+   unsigned char sha1[20];
+   unsigned char end[20];
+   struct object *a_obj, *b_obj;
+   unsigned 

Re: Limitiations of git rebase --preserve-merges --interactive

[Stefan Haller]

Anatoly Borodin  wrote:

> PS There are also some pieces of "what should work" in these tests:
> 
> t/t3409-rebase-preserve-merges.sh*
> t/t3410-rebase-preserve-dropped-merges.sh*
> t/t3411-rebase-preserve-around-merges.sh*
> t/t3414-rebase-preserve-onto.sh*

Thanks, this is interesting; I'm having trouble understanding the tests
though. Some of them use rebase -p -i, but I don't understand why they
use -i, or why that even works in a test (i.e. why it doesn't open an
editor).

In one test I saw "GIT_EDITOR=: git rebase -i -p", which I guess means
"use the initially given todo sheet unchanged". I don't see any tests
that do an interactive rebase and actually change the todo list.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

Re: Limitiations of git rebase --preserve-merges --interactive

[Stefan Haller]

Stefan Beller  wrote:

> On Thu, Sep 22, 2016 at 12:48 PM, Kevin Daudt  wrote:
> > On Thu, Sep 22, 2016 at 07:33:11PM +, Anatoly Borodin wrote:
> >> Hi Stefan,
> >>
> >> this section was added to the manual in the commit
> >> cddb42d2c58a9de9b2b5ef68817778e7afaace3e by "Jonathan Nieder"
> >>  6 years ago. Maybe he remembers better?
> >>
> >
> > Just to make it clear, this section explicitly talks about 'bugs' with
> > preserve-merges and interactive rebase.  Without the --preserve-merges
> > option, those operations works as expected.
> >
> > The reason, as that section explains, is that it's not possible to store
> > the merge structure in the flat todo list. I assume this means git
> > internally remembers where the merge commit was, and then restores it
> > while rebasing.
> >
> > Changing the order, or dropping commits might then give unexpected
> > results.
> >
> 
> The commit message may help as well:
> 
> rebase -i -p: document shortcomings
> 
> The rebase --preserve-merges facility presents a list of commits
> in its instruction sheet and uses a separate table to keep
> track of their parents.  Unfortunately, in practice this means
> that with -p after most attempts to rearrange patches, some
> commits have the "wrong" parent and the resulting history is
> rarely what the caller expected.
> 
> Yes, it would be nice to fix that.  But first, add a warning to the
> manual to help the uninitiated understand what is going on.

Thanks, but all of this still talks about the issues in very generic
terms ("most attempts to rearrange patches"). I'm interested in more
details as to exactly what kind of attempts do or don't work. In
particular, I'm interested in fixup/squash commands (without reordering
anything else), or dropping (non-merge) commits.

I could of course experiment with these and try to find out myself, but
I was hoping someone would just know the answer off the top of their
head, saving me some time.


-- 
Stefan Haller
Berlin, Germany
http://www.haller-berlin.de/

Re: [PATCH v2] gitweb: use highlight's shebang detection

[Ian Kelling]

On Thu, Sep 22, 2016, at 03:50 PM, Jakub Narębski wrote:
> W dniu 22.09.2016 o 00:18, Ian Kelling napisał:
>
> > The highlight binary can detect language by shebang when we can't tell
> > the syntax type by the name of the file. In that case, pass the blob
> > to "highlight --force" and the resulting html will have markup for
> > highlighting if the language was detected.
>
> This description feels a bit convoluted. Perhaps something like this:
>
>   The "highlight" binary can, in some cases, determine the language type
>   by the means of file contents, for example the shebang in the first
>   line
>   for some scripting languages.  Make use of this autodetection for files
>   which syntax is not known by gitweb.  In that case, pass the blob
>   contents to "highlight --force"; the parameter is needed to make it
>   always generate HTML output (which includes HTML-escaping).

Nice. Using it in v3.

>
> Also, we might want to have the information about performance of this
> solution either in the commit message, or in commit comments.

I tested it more rigorously and added to v3 commit message.

>
> >
> > Document the feature and improve syntax highlight documentation, add
> > test to ensure gitweb doesn't crash when language detection is used,
>
> All right.
>
> > and remove an unused parameter from gitweb_check_feature().
>
> First, that is guess_file_syntax(), not gitweb_check_feature().
> Second, this change could be made into independent patch, for example
> preparatory one.


Oops. I split it out in v3.

>
> >
> > Signed-off-by: Ian Kelling 
> > ---
> >  Documentation/gitweb.conf.txt  | 21 ++---
> >  gitweb/gitweb.perl | 14 +++---
> >  t/t9500-gitweb-standalone-no-errors.sh |  8 
> >  3 files changed, 29 insertions(+), 14 deletions(-)
> >
> > diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> > index a79e350..e632089 100644
> > --- a/Documentation/gitweb.conf.txt
> > +++ b/Documentation/gitweb.conf.txt
> > @@ -246,13 +246,20 @@ $highlight_bin::
> > Note that 'highlight' feature must be set for gitweb to actually
> > use syntax highlighting.
> >  +
> > -*NOTE*: if you want to add support for new file type (supported by
> > -"highlight" but not used by gitweb), you need to modify `%highlight_ext`
> > -or `%highlight_basename`, depending on whether you detect type of file
> > -based on extension (for example "sh") or on its basename (for example
> > -"Makefile").  The keys of these hashes are extension and basename,
> > -respectively, and value for given key is name of syntax to be passed via
> > -`--syntax ` to highlighter.
> > +*NOTE*: for a file to be highlighted, its syntax type must be detected
> > +and that syntax must be supported by "highlight".  The default syntax
> > +detection is minimal, and there are many supported syntax types with no
> > +detection by default.  There are three options for adding syntax
> > +detection.  The first and second priority are `%highlight_basename` and
> > +`%highlight_ext`, which detect based on basename (the full filename, for
> > +example "Makefile") and extension (for example "sh").  The keys of these
> > +hashes are the basename and extension, respectively, and the value for a
> > +given key is the name of the syntax to be passed via `--syntax `
> > +to "highlight".  The last priority is the "highlight" configuration of
> > +`Shebang` regular expressions to detect the language based on the first
> > +line in the file, (for example, matching the line "#!/bin/bash").  See
> > +the highlight documentation and the default config at
> > +/etc/highlight/filetypes.conf for more details.
> >  +
>
> I think the rewrite is a bit more readable.
>
> >  For example if repositories you are hosting use "phtml" extension for
> >  PHP files, and you want to have correct syntax-highlighting for those
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 33d701d..44094f4 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -3913,7 +3913,7 @@ sub blob_contenttype {
> >  # guess file syntax for syntax highlighting; return undef if no 
> > highlighting
> >  # the name of syntax can (in the future) depend on syntax highlighter used
> >  sub guess_file_syntax {
> > -   my ($highlight, $mimetype, $file_name) = @_;
> > +   my ($highlight, $file_name) = @_;
>
> Right.
>
> > return undef unless ($highlight && defined $file_name);
> > my $basename = basename($file_name, '.in');
> > return $highlight_basename{$basename}
> > @@ -3931,15 +3931,16 @@ sub guess_file_syntax {
> >  # or return original FD if no highlighting
> >  sub run_highlighter {
> > my ($fd, $highlight, $syntax) = @_;
> > -   return $fd unless ($highlight && defined $syntax);
> > +   return $fd unless ($highlight);
>
> Run highlighter if it is defined, even if gitweb doesn't know syntax,
> right.
>
> >
> > close $fd;
> > +   my $syntax_arg = (defined 

[PATCH v3 2/2] gitweb: use highlight's shebang detection

[Ian Kelling]

The "highlight" binary can, in some cases, determine the language type
by the means of file contents, for example the shebang in the first line
for some scripting languages.  Make use of this autodetection for files
which syntax is not known by gitweb.  In that case, pass the blob
contents to "highlight --force"; the parameter is needed to make it
always generate HTML output (which includes HTML-escaping).

Although we now run highlight on files which do not end up highlighted,
performance is virtually unaffected because when we call highlight, we
also call sanitize() instead of esc_html(), which is significantly
slower. After curling blob view of unhighlighted large and small text
files of perl code and license text 100 times each on a local
Apache/2.4.23 (Debian) instance, it's logs indicate +-1% difference in
request time for all file types.

Document the feature and improve syntax highlight documentation, add
test to ensure gitweb doesn't crash when language detection is used.

Signed-off-by: Ian Kelling 
---
 Documentation/gitweb.conf.txt  | 21 ++---
 gitweb/gitweb.perl | 10 +-
 t/t9500-gitweb-standalone-no-errors.sh |  8 
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index a79e350..e632089 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -246,13 +246,20 @@ $highlight_bin::
Note that 'highlight' feature must be set for gitweb to actually
use syntax highlighting.
 +
-*NOTE*: if you want to add support for new file type (supported by
-"highlight" but not used by gitweb), you need to modify `%highlight_ext`
-or `%highlight_basename`, depending on whether you detect type of file
-based on extension (for example "sh") or on its basename (for example
-"Makefile").  The keys of these hashes are extension and basename,
-respectively, and value for given key is name of syntax to be passed via
-`--syntax ` to highlighter.
+*NOTE*: for a file to be highlighted, its syntax type must be detected
+and that syntax must be supported by "highlight".  The default syntax
+detection is minimal, and there are many supported syntax types with no
+detection by default.  There are three options for adding syntax
+detection.  The first and second priority are `%highlight_basename` and
+`%highlight_ext`, which detect based on basename (the full filename, for
+example "Makefile") and extension (for example "sh").  The keys of these
+hashes are the basename and extension, respectively, and the value for a
+given key is the name of the syntax to be passed via `--syntax `
+to "highlight".  The last priority is the "highlight" configuration of
+`Shebang` regular expressions to detect the language based on the first
+line in the file, (for example, matching the line "#!/bin/bash").  See
+the highlight documentation and the default config at
+/etc/highlight/filetypes.conf for more details.
 +
 For example if repositories you are hosting use "phtml" extension for
 PHP files, and you want to have correct syntax-highlighting for those
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6cb4280..44094f4 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3931,15 +3931,16 @@ sub guess_file_syntax {
 # or return original FD if no highlighting
 sub run_highlighter {
my ($fd, $highlight, $syntax) = @_;
-   return $fd unless ($highlight && defined $syntax);
+   return $fd unless ($highlight);
 
close $fd;
+   my $syntax_arg = (defined $syntax) ? "--syntax $syntax" : "--force";
open $fd, quote_command(git_cmd(), "cat-file", "blob", $hash)." | ".
  quote_command($^X, '-CO', '-MEncode=decode,FB_DEFAULT', 
'-pse',
'$_ = decode($fe, $_, FB_DEFAULT) if !utf8::decode($_);',
'--', "-fe=$fallback_encoding")." | ".
  quote_command($highlight_bin).
- " --replace-tabs=8 --fragment --syntax $syntax |"
+ " --replace-tabs=8 --fragment $syntax_arg |"
or die_error(500, "Couldn't open file or run syntax 
highlighter");
return $fd;
 }
@@ -7063,8 +7064,7 @@ sub git_blob {
 
my $highlight = gitweb_check_feature('highlight');
my $syntax = guess_file_syntax($highlight, $file_name);
-   $fd = run_highlighter($fd, $highlight, $syntax)
-   if $syntax;
+   $fd = run_highlighter($fd, $highlight, $syntax);
 
git_header_html(undef, $expires);
my $formats_nav = '';
@@ -7117,7 +7117,7 @@ sub git_blob {
$line = untabify($line);
printf qq!%4i %s\n!,
   $nr, esc_attr(href(-replay => 1)), $nr, $nr,
-  $syntax ? sanitize($line) : esc_html($line, 
-nbsp=>1);
+  $highlight ? sanitize($line) : esc_html($line, 

[PATCH v3 1/2] gitweb: remove unused function parameter

[Ian Kelling]

Signed-off-by: Ian Kelling 
---
 gitweb/gitweb.perl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 33d701d..6cb4280 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3913,7 +3913,7 @@ sub blob_contenttype {
 # guess file syntax for syntax highlighting; return undef if no highlighting
 # the name of syntax can (in the future) depend on syntax highlighter used
 sub guess_file_syntax {
-   my ($highlight, $mimetype, $file_name) = @_;
+   my ($highlight, $file_name) = @_;
return undef unless ($highlight && defined $file_name);
my $basename = basename($file_name, '.in');
return $highlight_basename{$basename}
@@ -7062,7 +7062,7 @@ sub git_blob {
$have_blame &&= ($mimetype =~ m!^text/!);
 
my $highlight = gitweb_check_feature('highlight');
-   my $syntax = guess_file_syntax($highlight, $mimetype, $file_name);
+   my $syntax = guess_file_syntax($highlight, $file_name);
$fd = run_highlighter($fd, $highlight, $syntax)
if $syntax;
 
-- 
2.9.3

Re: [PATCH] run-command: async_exit no longer needs to be public

[Lars Schneider]

> On 22 Sep 2016, at 18:56, Ramsay Jones  wrote:
> 
> 
> Signed-off-by: Ramsay Jones 
> ---
> 
> Hi Lars,
> 
> If you need to re-roll your 'ls/filter-process' branch, could you please
> squash this into the relevant commit c42a4cbc ("run-command: move check_pipe()
> from write_or_die to run_command", 20-09-2016).
> 
> [Note that commit 9658846c ("write_or_die: handle EPIPE in async threads",
> 24-02-2016) introduced async_exit() specifically for use in the implementation
> of check_pipe(). Now that you have moved check_pipe() into run-command.c,
> it no longer needs to be public.]

Hi Ramsay,

thanks for noticing this. I actually hope that I don't need another re-roll :-)
If I don't re-roll. Should I make a patch with this cleanup or do you
take care of it?

Thanks,
Lars

Keychain access does not work under macOS Sierra

[Nicolas Vollmar]

It seems there were some changes at the keychain in macOS Sierra, after 
upgrading git seems not to be able to find the client certificate required to 
connect to our server.

fatal: unable to access 'https://xxx:8443/git/proj.git/': SSL: Can't find the 
certificate "John Doe" and its private key in the Keychain.

We would appreciate some hints how to address that.

Kind regards,
Nicolas

Re: [PATCH 1/2] ls-files: adding support for submodules

[Jeff King]

On Thu, Sep 22, 2016 at 10:47:17PM -0700, Stefan Beller wrote:

> On Thu, Sep 22, 2016 at 8:41 PM, Jeff King  wrote:
> 
> >>  * As Stefan alluded to (much) earlier, it might be a better idea
> >>to have these 'prefix' as the global option to "git" potty, not
> >>to each subcommand that happens to support them;
> >
> > That seems like it would be nice, but there's going to be an interim
> > period where some commands do not respect the global "--prefix" at all
> > (in the worst case, consider a third party command).
> 
> My current line of thinking is to have a new flag in command struct in
> git.c to enable the global --prefix, (c.f. RUN_SETUP | NEED_WORK_TREE)
> so we'd have a ALLOW_OUTSIDE_PREFIX flag which can be used to enable
> this feature. In case that flag is not set, but a user tries a
> --prefix=
> we can still
> 
> die("nope, we don't do that");

Yeah, a positive "I support this" flag would at least let us correctly
flag errors, which is the best we can do. That won't work for
non-builtins, but perhaps it is good enough in practice.

-Peff