Re: [PATCH v2 1/2] pkt-line: add packet_writel() and packet_read_line_gently()

[Torsten Bögershausen]

On 2017-03-24 16:27, Ben Peart wrote:
> Add packet_writel() which writes multiple lines in a single call and
> then calls packet_flush_gently(). Add packet_read_line_gently() to
> enable reading a line without dying on EOF.
> 
> Signed-off-by: Ben Peart 
> ---
>  pkt-line.c | 31 +++
>  pkt-line.h | 11 +++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/pkt-line.c b/pkt-line.c
> index d4b6bfe076..2788aa1af6 100644
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -171,6 +171,25 @@ int packet_write_fmt_gently(int fd, const char *fmt, ...)
>   return status;
>  }
>  
> +int packet_writel(int fd, const char *line, ...)
The name packet_writel is hard to distinguish from packet_write.
Would packet_write_lines make more sense ?

> +{
> + va_list args;
> + int err;
> + va_start(args, line);
> + for (;;) {
> + if (!line)
> + break;
> + if (strlen(line) > LARGE_PACKET_DATA_MAX)
> + return -1;
> + err = packet_write_fmt_gently(fd, "%s\n", line);
> + if (err)
> + return err;
> + line = va_arg(args, const char*);
> + }
> + va_end(args);
> + return packet_flush_gently(fd);
> +}
> +
I don't think that this va_start() is needed, even if it works.

int packet_write_line(int fd, const char *lines[])
|
const char *line = *lines;
int err;
while (line) {
if (strlen(line) > LARGE_PACKET_DATA_MAX)
return -1;
err = packet_write_fmt_gently(fd, "%s\n", line);
if (err)
return err;
lines++;
line = *lines;
}
return packet_flush_gently(fd);
]

Re: Re: Re: GSoC Project | Convert interactive rebase to C

[Inaw Tham]

Johannes Schindelin  wrote:
> On Tue, 21 Mar 2017, Ivan Tham wrote:
> > Stefan Beller  wrote:
> > > On Mon, Mar 20, 2017 at 9:41 AM, Ivan Tham  wrote:
> > > > I am Ivan Tham. Currently studying in Computer Science in APIIT
> > > > Malaysia. I am interested particapate in Google Summer of Code 2017
> > > > under git organization. I would like to attempt "Add more builtin
> > > > patterns for userdiff" particularly for shell for my microproject.
> > >
> > > I'd love to see proper shell support!  Although there is already some
> > > support for shell (by looking at diffs on our test suite) ? So I am
> > > not sure what there is left to do? Can you clarify what you're trying
> > > there?
> > 
> > Are you sure about that? From what I had looked into userdiff.c, there
> > is no support for shell. There just a recent patch for [go patterns][0].
> > Or perhaps I should have rename it as "userdiff.c: patterns for "shell"
> > language"?
> 
> I also could not find any shell patterns in the userdiff code...

Thanks a lot for replying me, I thought no one was intereted. :D

> > > > I am interested to work on "Convert interactive rebase to C"
> > >
> > > +cc Johannes, who recently worked on rebase and the sequencer.
> 
> Glad you are interested! Please note that large parts of the interactive
> rebase are already in C now, but there is enough work left in that corner.

Glad to hear that, I would really like to see interactive rebase in C.

> > > > aiming to port most builtins stuff to C in which we can reduce the
> > > > size of git. Additionally, I would also like to convert scripts to
> > > > builtins as an additional milestone.
> 
> Careful. It is a ton of work to get the rebase -i conversion done, and
> then a ton of work to get it integrated. That will fill 3 months, very
> easily.

My main aim is to reduce the extra dependency of perl, but planning to start
with rebase, can I make that an optional task where I can help out after I
had completed my main task during gsoc?

> > > > What do you think of these projects? Would it collide with Valery
> > > > Tolstov's Shell to Builtins proposal?
> 
> I missed that proposal, and could only find submodule-related mails on the
> public-inbox server. Care to provide a pointer?
> 
> > > Curious why all people ask about colliding with Valerys proposal here?
> > > I do not think it would collide, as submodules and rebase are very
> > > different areas of the code base.
> 
> Indeed ;-)

Cheers,
Ivan Tham

Re: [PATCH v7 0/7] short status: improve reporting for submodule changes

[Jonathan Nieder]

Stefan Beller wrote:

> v7:
> * taken all of Jonathan minor nits, so patch 1..6 should be good to go
> * patch 7 lacks tests and documentation (according to Jonathan...)
>   but as it is the last patch, just fixing a minor detail we can leave it off.
>
> Junio, please take patch 1-6 as usual, I will be out until next Wednesday.
[...]
> Stefan Beller (8):
>   submodule.c: port is_submodule_modified to use porcelain 2
>   submodule.c: use argv_array in is_submodule_modified
>   submodule.c: convert is_submodule_modified to use
> strbuf_getwholeline_fd
>   submodule.c: port is_submodule_modified to use porcelain 2
>   submodule.c: factor out early loop termination in
> is_submodule_modified
>   submodule.c: stricter checking for submodules in is_submodule_modified
>   short status: improve reporting for submodule changes
>   submodule.c: correctly handle nested submodules in
> is_submodule_modified
>
>  Documentation/git-status.txt |  9 +++
>  submodule.c  | 56 ---
>  t/t3600-rm.sh| 18 ++
>  t/t7506-status-submodule.sh  | 57 
> 
>  wt-status.c  | 13 --
>  5 files changed, 116 insertions(+), 37 deletions(-)

Patches 1-6 are

Reviewed-by: Jonathan Nieder 

The effect of patch 7 on --porcelain=2 output is subtle enough that I
don't feel I understand it.  I think it heads in a good direction but
indeed, some tests could help to illustrate the desired behavior.

Thanks for your patient work.
Jonathan

Re: [PATCH] [GSoC] remove_subtree(): reimplement using iterators

[Daniel Ferreira (theiostream)]

> On Fri, Mar 24, 2017 at 2:02 PM, Stefan Beller  wrote:

> Welcome to the Git community!

Thank you!

> Please use a more imperative style. (e.g. s/Uses/Use/ ...
> s/and simplfying/which simplifies/)

Thank you. Will do in a second version of this patch.

> Thanks for this link. It gives good context for reviewing the change,
> but it will not be good context to record as a commit message.
> (When someone looks at a commit message later on, they are usually trying
> to figure out what the author was thinking; if there were any special cases to
> be thought about. Was performance on the authors mind? etc)

> So I propose to put the link into the more informal section if a
> reroll is needed.

Perfect. I will remove it from the message.

> Instead of constructing the path again here based on relative path
> and the path parameter, I wonder if we could use
>
> if (unlink(diter->path))
> ..
>
> here? Then we would not need the strbuf at all?

Yes, we can! Thank you for the pointer. Will be in the next version of the
patch.

> Also we'd need to handle (empty) directories differently for removal?

>From what I've tested, we do not need to do it.

> Do we need to check the return code of dir_iterator_advance
> for ITER_ERROR as well?

I believe not – it only tries to perform an operation if we have ITER_OK. Since
ITER_ERROR would end up in a no-op anyway I don't see how a check for it
would be useful.

>
>
> > }
> > -   closedir(dir);
> > +
> > if (rmdir(path->buf))
> > die_errno("cannot rmdir '%s'", path->buf);
>
> This would remove the "top level" directory as given by path.
> When reading the dir-iterator code, I am not sure if this is
> also part of the yield in dir_iterator_advance.

I've tested it, and it does not yield in there.

Thank you for the advice, and as stated, will submit a v2 of the patch
in short notice.

Thank you,
Daniel.

[PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

When a nested submodule has untracked files, it would be reported as
"modified submodule" in the superproject, because submodules are not
parsed correctly in is_submodule_modified as they are bucketed into
the modified pile as "they are not an untracked file".

Signed-off-by: Stefan Beller 
---
 submodule.c | 23 +--
 t/t3600-rm.sh   |  2 +-
 t/t7506-status-submodule.sh |  2 +-
 3 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index fa21c7bb72..730cc9513a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1078,8 +1078,27 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
/* regular untracked files */
if (buf.buf[0] == '?')
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-   else
-   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+
+   if (buf.buf[0] == 'u' ||
+   buf.buf[0] == '1' ||
+   buf.buf[0] == '2') {
+   /*
+* T XY :
+* T = line type, XY = status,  = submodule state
+*/
+   if (buf.len < 1 + 1 + 2 + 1 + 4)
+   die("BUG: invalid status --porcelain=2 line %s",
+   buf.buf);
+
+   /* regular unmerged and renamed files */
+   if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
+   /* nested untracked file */
+   dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+
+   if (memcmp(buf.buf + 5, "S..U", 4))
+   /* other change */
+   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+   }
 
if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule 
with nested untracked fi
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified_inside actual &&
+   test_cmp expect.modified_untracked actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 6d3acb4a5a..ab822c79e6 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -340,7 +340,7 @@ test_expect_success 'status with untracked file in nested 
submodule (porcelain)'
 test_expect_success 'status with untracked file in nested submodule (short)' '
git -C super status --short >output &&
diff output - <<-\EOF
-m sub1
+? sub1
EOF
 '
 
-- 
2.12.0.rc1.49.gdeb397943c.dirty

[PATCH v7 0/7] short status: improve reporting for submodule changes

[Stefan Beller]

v7:
* taken all of Jonathan minor nits, so patch 1..6 should be good to go
* patch 7 lacks tests and documentation (according to Jonathan...)
  but as it is the last patch, just fixing a minor detail we can leave it off.

Junio, please take patch 1-6 as usual, I will be out until next Wednesday.


Thanks,
Stefan

v6:
* kill the child once we know all information that we ask for, as an 
optimization
* reordered the patches for that
* strbuf_getwholeline instead of its _fd version.

v5:
* fixed rebase error in the first 2 patches
* the last 3 patches introduce behavior change outside the scope of 
is_modified_submodule
  whereas the first 4 ought to just be local changes.
  
Thanks,
Stefan

v4:
* broken down in a lot of tiny patches.

Jonathan wrote:
> Patch 1/3 is the one that gives me pause, since I hadn't been able to
> chase down all callers.  Would it be feasible to split that into two:
> one patch to switch to --porcelain=2 but not change behavior, and a
> separate patch to improve what is stored in the dirty_submodule flags?

This part is in the latest patch now.

Thanks,
Stefan


v3:
This comes as a series; first I'd like to refactor is_submodule_modified
to take advantage of the new porcelain=2 plumbing switch to check for changes
in the submodule.

On top of the refactoring comes the actual change, which moved the
rewriting of the submodule change indicator letter to the collection part.

Thanks,
Stefan

Stefan Beller (8):
  submodule.c: port is_submodule_modified to use porcelain 2
  submodule.c: use argv_array in is_submodule_modified
  submodule.c: convert is_submodule_modified to use
strbuf_getwholeline_fd
  submodule.c: port is_submodule_modified to use porcelain 2
  submodule.c: factor out early loop termination in
is_submodule_modified
  submodule.c: stricter checking for submodules in is_submodule_modified
  short status: improve reporting for submodule changes
  submodule.c: correctly handle nested submodules in
is_submodule_modified

 Documentation/git-status.txt |  9 +++
 submodule.c  | 56 ---
 t/t3600-rm.sh| 18 ++
 t/t7506-status-submodule.sh  | 57 
 wt-status.c  | 13 --
 5 files changed, 116 insertions(+), 37 deletions(-)

-- 
2.12.1.438.gb674c4c09c

[PATCH 4/7] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

Migrate 'is_submodule_modified' to the new porcelain format of
git-status. This conversion attempts to convert faithfully, i.e.
the behavior ought to be exactly the same.

As the output in the parsing only distinguishes between untracked files
and the rest, this is easy to port to the new format, as we only
need to identify untracked files and the rest is handled in the "else"
case.

untracked files are indicated by only a single question mark instead of
two question marks, so the conversion is easy.

Signed-off-by: Stefan Beller 
Reviewed-by: Jonathan Nieder 
---
 submodule.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index e72781d9f2..5865795b9f 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1060,7 +1060,7 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
}
strbuf_reset();
 
-   argv_array_pushl(, "status", "--porcelain", NULL);
+   argv_array_pushl(, "status", "--porcelain=2", NULL);
if (ignore_untracked)
argv_array_push(, "-uno");
 
@@ -1070,11 +1070,12 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
cp.out = -1;
cp.dir = path;
if (start_command())
-   die("Could not run 'git status --porcelain' in submodule %s", 
path);
+   die("Could not run 'git status --porcelain=2' in submodule %s", 
path);
 
fp = xfdopen(cp.out, "r");
while (strbuf_getwholeline(, fp, '\n') != EOF) {
-   if ((buf.buf[0] == '?') && (buf.buf[1] == '?'))
+   /* regular untracked files */
+   if (buf.buf[0] == '?')
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
else
dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
@@ -1093,7 +1094,7 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
fclose(fp);
 
if (finish_command() && !ignore_cp_exit_code)
-   die("'git status --porcelain' failed in submodule %s", path);
+   die("'git status --porcelain=2' failed in submodule %s", path);
 
strbuf_release();
return dirty_submodule;
-- 
2.12.0.rc1.49.gdeb397943c.dirty

[PATCH 2/7] submodule.c: factor out early loop termination in is_submodule_modified

[Stefan Beller]

This makes it easier for a follow up patch.

Signed-off-by: Stefan Beller 
Reviewed-by: Jonathan Nieder 
---
 submodule.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 2c667ac95a..93e3fefd39 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1075,16 +1075,16 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
len = strbuf_read(, cp.out, 1024);
line = buf.buf;
while (len > 2) {
-   if ((line[0] == '?') && (line[1] == '?')) {
+   if ((line[0] == '?') && (line[1] == '?'))
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-   if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-   break;
-   } else {
+   else
dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-   if (ignore_untracked ||
-   (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
-   break;
-   }
+
+   if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
+   ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
+ignore_untracked))
+   break;
+
next_line = strchr(line, '\n');
if (!next_line)
break;
-- 
2.12.0.rc1.49.gdeb397943c.dirty

[PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline

[Stefan Beller]

Instead of implementing line reading yet again, make use of our beautiful
library function to read one line.  By using strbuf_getwholeline instead
of strbuf_read, we avoid having to allocate memory for the entire child
process output at once.  That is, we limit maximum memory usage.
Also we can start processing the output as it comes in, no need to
wait for all of it.

Once we know all information that we care about, we can terminate
the child early. In that case we do not care about its exit code as well.
By just closing our side of the pipe the child process will get a SIGPIPE
signal, which it will not report nor do we report it in finish_command,
ac78663b0d (run-command: don't warn on SIGPIPE deaths, 2015-12-29).

Helped-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 submodule.c | 30 ++
 t/t7506-status-submodule.sh | 18 +-
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/submodule.c b/submodule.c
index 93e3fefd39..e72781d9f2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1041,12 +1041,12 @@ int fetch_populated_submodules(const struct argv_array 
*options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-   ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
+   FILE *fp;
unsigned dirty_submodule = 0;
-   const char *line, *next_line;
const char *git_dir;
+   int ignore_cp_exit_code = 0;
 
strbuf_addf(, "%s/.git", path);
git_dir = read_gitfile(buf.buf);
@@ -1072,29 +1072,27 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
if (start_command())
die("Could not run 'git status --porcelain' in submodule %s", 
path);
 
-   len = strbuf_read(, cp.out, 1024);
-   line = buf.buf;
-   while (len > 2) {
-   if ((line[0] == '?') && (line[1] == '?'))
+   fp = xfdopen(cp.out, "r");
+   while (strbuf_getwholeline(, fp, '\n') != EOF) {
+   if ((buf.buf[0] == '?') && (buf.buf[1] == '?'))
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
else
dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 
if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
-ignore_untracked))
-   break;
-
-   next_line = strchr(line, '\n');
-   if (!next_line)
+ignore_untracked)) {
+   /*
+* We're not interested in any further information from
+* the child any more, neither output nor its exit code.
+*/
+   ignore_cp_exit_code = 1;
break;
-   next_line++;
-   len -= (next_line - line);
-   line = next_line;
+   }
}
-   close(cp.out);
+   fclose(fp);
 
-   if (finish_command())
+   if (finish_command() && !ignore_cp_exit_code)
die("'git status --porcelain' failed in submodule %s", path);
 
strbuf_release();
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34da83..51f8d0d034 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -177,8 +177,24 @@ test_expect_success 'status with added file in modified 
submodule with .git file
test_i18ngrep "modified:   sub (new commits, modified content)" output
 '
 
+test_expect_success 'status with a lot of untracked files in the submodule' '
+   (
+   cd sub
+   i=0 &&
+   while test $i -lt 1024
+   do
+   >some-file-$i
+   i=$(( $i + 1 ))
+   done
+   ) &&
+   git status --porcelain sub 2>err.actual &&
+   test_must_be_empty err.actual &&
+   rm err.actual
+'
+
 test_expect_success 'rm submodule contents' '
-   rm -rf sub/* sub/.git
+   rm -rf sub &&
+   mkdir sub
 '
 
 test_expect_success 'status clean (empty submodule dir)' '
-- 
2.12.0.rc1.49.gdeb397943c.dirty

[PATCH 1/7] submodule.c: use argv_array in is_submodule_modified

[Stefan Beller]

struct argv_array is easier to use and maintain.

Signed-off-by: Stefan Beller 
Reviewed-by: Jonathan Nieder 
---
 submodule.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..2c667ac95a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1043,12 +1043,6 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
 {
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
-   const char *argv[] = {
-   "status",
-   "--porcelain",
-   NULL,
-   NULL,
-   };
struct strbuf buf = STRBUF_INIT;
unsigned dirty_submodule = 0;
const char *line, *next_line;
@@ -1066,10 +1060,10 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
}
strbuf_reset();
 
+   argv_array_pushl(, "status", "--porcelain", NULL);
if (ignore_untracked)
-   argv[2] = "-uno";
+   argv_array_push(, "-uno");
 
-   cp.argv = argv;
prepare_submodule_repo_env(_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
-- 
2.12.0.rc1.49.gdeb397943c.dirty

[PATCH 6/7] short status: improve reporting for submodule changes

[Stefan Beller]

If I add an untracked file to a submodule or modify a tracked file,
currently "git status --short" treats the change in the same way as
changes to the current HEAD of the submodule:

$ git clone --quiet --recurse-submodules 
https://gerrit.googlesource.com/gerrit
$ echo hello >gerrit/plugins/replication/stray-file
$ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
$ git -C gerrit status --short
 M plugins/replication

This is by analogy with ordinary files, where "M" represents a change
that has not been added yet to the index.  But this change cannot be
added to the index without entering the submodule, "git add"-ing it,
and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this.  These are similar
to the existing "??" and " M" but mean that the submodule (not the
parent project) has new untracked files and modified files, respectively.
The user can use "git add" and "git commit" from within the submodule to
add them.

Changes to the submodule's HEAD commit can be recorded in the index with
a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for
the submodule.  They represent increasing levels of change --- the last
one that applies is shown (e.g., " m" if there are both modified files
and untracked files in the submodule, or " M" if the submodule's HEAD
has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain
level 1, which shares code with "status --short".  We only change
"git status --short".

Non-short "git status" and "git status --porcelain=2" already handle
these cases by showing more detail:

$ git -C gerrit status --porcelain=2
1 .M S.MU 16 16 16 305c864db28eb0c77c8499bc04c87de3f849cf3c 
305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
$ git -C gerrit status
[...]
modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

Helped-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
Reviewed-by: Jonathan Nieder 
---
 Documentation/git-status.txt |  9 +++
 t/t3600-rm.sh| 18 ++
 t/t7506-status-submodule.sh  | 57 
 wt-status.c  | 17 +++--
 4 files changed, 94 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..01b457c322 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -181,6 +181,13 @@ in which case `XY` are `!!`.
 !   !ignored
 -
 
+Submodules have more state and instead report
+   Mthe submodule has a different HEAD than
+recorded in the index
+   mthe submodule has modified content
+   ?the submodule has untracked files
+
+
 If -b is used the short-format status is preceded by a line
 
 ## branchname tracking info
@@ -210,6 +217,8 @@ field from the first filename).  Third, filenames 
containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single 
`?`.
+
 Porcelain Format Version 2
 ~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..a6e5c5bd56 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -268,6 +268,14 @@ cat >expect.modified actual &&
@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with 
untracked files fails unle
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified actual &&
+   test_cmp expect.modified_untracked actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule 
with different nested HE
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified actual &&
+   test_cmp expect.modified_inside actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule 
with nested modification

[PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified

[Stefan Beller]

By having a stricter check in the superproject we catch errors earlier,
instead of spawning a child process to tell us.

Reviewed-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 5865795b9f..fa21c7bb72 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1052,11 +1052,12 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
git_dir = read_gitfile(buf.buf);
if (!git_dir)
git_dir = buf.buf;
-   if (!is_directory(git_dir)) {
+   if (!is_git_directory(git_dir)) {
+   if (is_directory(git_dir))
+   die(_("'%s' not recognized as a git repository"), 
git_dir);
strbuf_release();
/* The submodule is not checked out, so it is not modified */
return 0;
-
}
strbuf_reset();
 
-- 
2.12.0.rc1.49.gdeb397943c.dirty

Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

On Fri, Mar 24, 2017 at 4:31 PM, Jonathan Nieder  wrote:
>
> Can this overflow the buffer?  Submodule state is supposed to be 4
> characters, so could do
>
> /*
>  * T XY :
>  * T = line type, XY = status,  = submodule state
>  */
> if (buf.len < 1 + 1 + 2 + 1 + 4)
> die("BUG: invalid status --porcelain=2 line 
> %s",
> buf.buf);
>
> if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
> /* untracked file */
> dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
>
> if (memcmp(buf.buf + 5, "S..U", 4))
> /* other change */
> dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;

good idea, will use.


> }
>
>> + /* nested submodule handling */
>> + if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
>> + dirty_submodule |= 
>> DIRTY_SUBMODULE_MODIFIED;
>> + if (buf.buf[8] == 'U')
>> + dirty_submodule |= 
>> DIRTY_SUBMODULE_UNTRACKED;
>> + } else
>> + dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
>> + }
>
> I get lost in these cases. What does it mean if we see S..., for
> example?

That means there is a submodule with no new commits, no modified files
and no untracked files. ("Why did we even output this?", oh because of
it is in either 2 (moved) or because the hashes are different, i.e.
we already added the new HEAD of the submodule)

>
> Some tests covering these cases with --porcelain=2 and a brief mention
> in documentation may help.

ok, will do.

Re: [PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline

[Stefan Beller]

On Fri, Mar 24, 2017 at 3:38 PM, Jonathan Nieder  wrote:
> It also overlaps work a little better.

mentioned

>> Once we know all information that we care about, we can terminate
>> the child early. In that case we do not care about its exit code as well.
>
> Should this say something about SIGPIPE?

done

> language nit: s/, no output/: neither output/

done

> wait_or_whine(cp->pid, cp->argv[0], 1) doesn't do that but is meant for
> signal handling.  Maybe we should rely on SIGPIPE instead (which
> wait_or_whine always silences) and avoid the kill() call.

done

> Can there be a test for this case (i.e. having lots of untracked files
> in the submodule so the child process fills its pipe buffer and has to
> exit early)?

done

Re: [PATCH v2 2/3] sequencer: make commit options more extensible

[Johannes Schindelin]

Hi Junio,

On Thu, 23 Mar 2017, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > @@ -926,14 +930,14 @@ static void record_in_rewritten(struct object_id *oid,
> >  static int do_pick_commit(enum todo_command command, struct commit *commit,
> > struct replay_opts *opts, int final_fixup)
> >  {
> > -   int edit = opts->edit, cleanup_commit_message = 0;
> > -   const char *msg_file = edit ? NULL : git_path_merge_msg();
> > +   unsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;
> > +   const char *msg_file = opts->edit ? NULL : git_path_merge_msg();
> > unsigned char head[20];
> > struct commit *base, *next, *parent;
> > const char *base_label, *next_label;
> > struct commit_message msg = { NULL, NULL, NULL, NULL };
> > struct strbuf msgbuf = STRBUF_INIT;
> > -   int res, unborn = 0, amend = 0, allow = 0;
> > +   int res, unborn = 0;
> > ... 
> > @@ -1123,11 +1127,11 @@ static int do_pick_commit(enum todo_command 
> > command, struct commit *commit,
> > if (allow < 0) {
> > res = allow;
> > goto leave;
> > -   }
> > +   } else if (allow)
> > +   flags |= ALLOW_EMPTY;
> 
> Making "flags" unsigned was a correct change, but this is now wrong,
> as "allow" is made unsigned by accident.

Right. Bummer.

> Perhaps something like this needs to be squashed in?
> 
>  sequencer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index bc2fe48e65..6c423566e6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -932,14 +932,14 @@ static void record_in_rewritten(struct object_id *oid,
>  static int do_pick_commit(enum todo_command command, struct commit *commit,
>   struct replay_opts *opts, int final_fixup)
>  {
> - unsigned int flags = opts->edit ? EDIT_MSG : 0, allow = 0;
> + unsigned int flags = opts->edit ? EDIT_MSG : 0;
>   const char *msg_file = opts->edit ? NULL : git_path_merge_msg();
>   unsigned char head[20];
>   struct commit *base, *next, *parent;
>   const char *base_label, *next_label;
>   struct commit_message msg = { NULL, NULL, NULL, NULL };
>   struct strbuf msgbuf = STRBUF_INIT;
> - int res, unborn = 0;
> + int allow, res, unborn = 0;

I had originally moved it from there, to stay with the related flags, but
I should just have left it there.

Your patch looks good, you could do even better by reverting that move
(IIRC it was at the end of the line, and it was set to 0 by default).

Ciao,
Dscho

Re: What's cooking in git.git (Mar 2017, #10; Fri, 24)

[Brandon Williams]

On 03/24, Junio C Hamano wrote:
> * bw/recurse-submodules-relative-fix (2017-03-17) 5 commits
>  - ls-files: fix bug when recursing with relative pathspec
>  - ls-files: fix typo in variable name
>  - grep: fix bug when recursing with relative pathspec
>  - setup: allow for prefix to be passed to git commands
>  - grep: fix help text typo
> 
>  A few commands that recently learned the "--recurse-submodule"
>  option misbehaved when started from a subdirectory of the
>  superproject.

Anything more you think needs to be done about this?  I noticed that
Dscho's config series hit master so I could rebase against that (as
there is a small conflict).  Aside from that it didn't seem like there
were many complaints with the proposed fix.

-- 
Brandon Williams

Will OpenSSL's license change impact us?

[Ævar Arnfjörð Bjarmason]

They're changing their license[1] to Apache 2 which unlike the current
fuzzy compatibility with the current license[2] is explicitly
incompatible with GPLv2[3].

We use OpenSSL for SHA1 by default unless NO_OPENSSL=YesPlease.

This still hasn't happened, but given the lifetime of git versions
packaged up by distros knowing sooner than later if this is going to
be a practical problem would be good.

If so perhaps we could copy the relevant subset of the code int our
tree, or libressl's, or improve block-sha1.

We also use OpenSSL for git-imap-send, AFAICT with no fallback other
than "don't use ssl" or "use stunnel".

1. https://www.openssl.org/blog/blog/2017/03/20/license/
2. https://www.openssl.org/docs/faq.html#LEGAL2
3. https://www.apache.org/licenses/GPL-compatibility.html

Re: [PATCH v1] travis-ci: build and test Git on Windows

[Johannes Schindelin]

Hi Peff,

On Thu, 23 Mar 2017, Jeff King wrote:

> My pattern is particularly spiky from Travis's perspective, because once
> a day I rebase everything on top of master and push them the whole thing
> in a bunch. So they 75 branches, all at once. That seems like it would
> be ripe for throttling (though I would much rather they just queue the
> builds and do them one at a time).

Travis is optimized for Continuous Integration, i.e. one or maybe two
integration branches that merge PRs every once in a while.

What we would need is Continuous Testing, really, because we do not do
Continuous Integration at all.

On the point of the sekrit variable: all I tried to do is to avoid
overwhelming the (currently) single VM with plenty of jobs. If the build &
test would not take so darned long on Windows due to going in and out of
the POSIX emulation layer a gazillion times (caused by intense shell
scripting), the tests could be faster. But that would require a massive
amount of work, and I simply have too much on my plate to take that task
on in addition.

If you have an idea how to prevent the overloading of the VM by any means
that does not involve that token, please tell me.

Ciao,
Dscho

Re: [PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

> When a nested submodule has untracked files, it would be reported as
> "modified submodule" in the superproject, because submodules are not
> parsed correctly in is_submodule_modified as they are bucketed into
> the modified pile as "they are not an untracked file".
> 
> Signed-off-by: Stefan Beller 
> ---
>  submodule.c | 16 ++--
>  t/t3600-rm.sh   |  2 +-
>  t/t7506-status-submodule.sh |  2 +-
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/submodule.c b/submodule.c
> index 467f1de763..ec7e9b1b06 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1078,8 +1078,20 @@ unsigned is_submodule_modified(const char *path, int 
> ignore_untracked)
>   /* regular untracked files */
>   if (buf.buf[0] == '?')
>   dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
> - else
> - dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
> +
> + /* regular unmerged and renamed files */
> + if (buf.buf[0] == 'u' ||
> + buf.buf[0] == '1' ||
> + buf.buf[0] == '2') {
> + if (buf.buf[5] == 'S') {

Can this overflow the buffer?  Submodule state is supposed to be 4
characters, so could do

/*
 * T XY :
 * T = line type, XY = status,  = submodule state
 */
if (buf.len < 1 + 1 + 2 + 1 + 4)
die("BUG: invalid status --porcelain=2 line %s",
buf.buf);

if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
/* untracked file */
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;

if (memcmp(buf.buf + 5, "S..U", 4))
/* other change */
dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
}

> + /* nested submodule handling */
> + if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
> + dirty_submodule |= 
> DIRTY_SUBMODULE_MODIFIED;
> + if (buf.buf[8] == 'U')
> + dirty_submodule |= 
> DIRTY_SUBMODULE_UNTRACKED;
> + } else
> + dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
> + }

I get lost in these cases. What does it mean if we see S..., for
example?

As an example, if I am understanding correctly, before this patch, if
I have a submodule-in-submodule:

$ find . -name .git
.git
sub/.git
sub/subsub/.git

and I put a stray file in the sub-sub module:

$ echo stray-file >sub/subsub/x.o

then status --porcelain=2 tells me that sub is modified:

$ git status --porcelain=2
1 .M S.M. [...] sub

What should it say after the patch?  Is the XY field ".." or ".M"?

Some tests covering these cases with --porcelain=2 and a brief mention
in documentation may help.

Thanks,
Jonathan

diff --git i/submodule.c w/submodule.c
index ec7e9b1b06..aec1b2cdca 100644
--- i/submodule.c
+++ w/submodule.c
@@ -1083,13 +1083,18 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
if (buf.buf[0] == 'u' ||
buf.buf[0] == '1' ||
buf.buf[0] == '2') {
-   if (buf.buf[5] == 'S') {
-   /* nested submodule handling */
-   if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
-   dirty_submodule |= 
DIRTY_SUBMODULE_MODIFIED;
-   if (buf.buf[8] == 'U')
-   dirty_submodule |= 
DIRTY_SUBMODULE_UNTRACKED;
-   } else
+   /*
+* T XY :
+* T = line type, XY = status,  = submodule state
+*/
+   if (buf.len < 1 + 1 + 2 + 1 + 4)
+   die("BUG: invalid status --porcelain=2 line %s",
+   buf.buf);
+   if (buf.buf[5] == 'S' && buf.buf[8] == 'U')
+   /* untracked file */
+   dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
+   if (memcmp(buf.buf + 5, "S..U", 4))
+   /* other change */
dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
}
 

[PATCH 6/7] mingw: enable DC_AND_OPENSSL_SHA1 by default

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 config.mak.uname | 1 +
 1 file changed, 1 insertion(+)

diff --git a/config.mak.uname b/config.mak.uname
index 399fe192719..a16e9ef0551 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -552,6 +552,7 @@ else
USE_LIBPCRE= YesPlease
NO_CURL =
USE_NED_ALLOCATOR = YesPlease
+   DC_AND_OPENSSL_SHA1 = YesPlease
else
COMPAT_CFLAGS += -D__USE_MINGW_ANSI_STDIO
NO_CURL = YesPlease
-- 
2.12.1.windows.1

[PATCH 7/7] p0013: new test to compare SHA1DC vs OpenSSL

[Johannes Schindelin]

To demonstrate the need for the core.enableSHA1DC knob, this test
compares the performance of the SHA-1 algorithms with collision
detection vs OpenSSL's (that does not detect attempted collision
attacks).

The payload size of 300MB was actually not concocted from thin air, but
is based on the massive Windows monorepo, whose index file weighs in
with roughly that size (and which is SHA-1'ed upon every single read and
write).

On this developer's machine, this comparison shows a hefty difference:

0013.1: calculate SHA-1 for 300MB (SHA1DC)3.03(0.03+0.17)
0013.2: calculate SHA-1 for 300MB (OpenSSL)   0.58(0.06+0.16)

It is not only that ~6x slower performance is a pretty tall order, the
absolute numbers themselves speak a very clear language: having to wait
one second every time a file is `git add`ed is noticeable, but one can
handwave it away. Having to wait six seconds (3 to read the index, a
fraction of a millisecond to hash the new contents and update the
in-memory index, then 3 seconds to write out the index) is outright
annoying. And unnecessary, too: the content of the index is never
crafted to cause SHA-1 collisions.

Obviously, this test requires that Git was built with the new
DC_AND_OPENSSL_SHA1 make flag; it is skipped otherwise.

Signed-off-by: Johannes Schindelin 
---
 t/perf/p0013-sha1dc.sh | 23 +++
 1 file changed, 23 insertions(+)
 create mode 100644 t/perf/p0013-sha1dc.sh

diff --git a/t/perf/p0013-sha1dc.sh b/t/perf/p0013-sha1dc.sh
new file mode 100644
index 000..e08473ac969
--- /dev/null
+++ b/t/perf/p0013-sha1dc.sh
@@ -0,0 +1,23 @@
+#!/bin/sh
+
+test_description="Tests performance of SHA1DC vs OpenSSL"
+
+. ./perf-lib.sh
+
+test -n "$DC_AND_OPENSSL_SHA1" || {
+   skip_all='DC_AND_OPENSSL_SHA1 required for this test'
+   test_done
+}
+
+test_perf 'calculate SHA-1 for 300MB (SHA1DC)' '
+   dd if=/dev/zero bs=1M count=300 |
+   test-sha1
+'
+
+test_perf 'calculate SHA-1 for 300MB (OpenSSL)' '
+   dd if=/dev/zero bs=1M count=300 |
+   test-sha1 --disable-sha1dc
+'
+
+test_done
+
-- 
2.12.1.windows.1

Re: What's cooking in git.git (Mar 2017, #10; Fri, 24)

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 24, 2017 at 10:21 PM, Junio C Hamano  wrote:

> * ab/test-readme-updates (2017-03-23) 4 commits
>  - SQUASH???
>  - t/README: clarify the test_have_prereq documentation
>  - t/README: change "Inside  part" to "Inside the  part"
>  - t/README: link to metacpan.org, not search.cpan.org
>
>  Doc updates.
>
>  Waiting for a reaction to SQUASH???

Sorry about the late reply. That squash looks good to me, please squash it in.

> * ab/doc-submitting (2017-03-21) 3 commits
>  - SQUASH??? remove "alias" thing
>  - doc/SubmittingPatches: show how to get a CLI commit summary
>  - doc/SubmittingPatches: clarify the casing convention for "area: change..."
>
>  Doc update.
>
>  Any further comments?

Squashing that in looks good.

> * ab/ref-filter-no-contains (2017-03-24) 16 commits
>  - tag: add tests for --with and --without
>  - ref-filter: reflow recently changed branch/tag/for-each-ref docs
>  - ref-filter: add --no-contains option to tag/branch/for-each-ref
>  - tag: change --point-at to default to HEAD
>  - tag: implicitly supply --list given another list-like option
>  - tag: change misleading --list  documentation
>  - parse-options: add OPT_NONEG to the "contains" option
>  - tag: add more incompatibles mode tests
>  - for-each-ref: partly change  to  in help
>  - tag tests: fix a typo in a test description
>  - tag: remove a TODO item from the test suite
>  - ref-filter: add test for --contains on a non-commit
>  - ref-filter: make combining --merged & --no-merged an error
>  - tag doc: reword --[no-]merged to talk about commits, not tips
>  - tag doc: split up the --[no-]merged documentation
>  - tag doc: move the description of --[no-]merged earlier
>
>  "git tag/branch/for-each-ref" family of commands long allowed to
>  filter the refs by "--contains X" (show only the refs that are
>  descendants of X), "--merged X" (show only the refs that are
>  ancestors of X), "--no-merged X" (show only the refs that are not
>  ancestors of X).  One curious omission, "--no-contains X" (show
>  only the refs that are not descendants of X) has been added to
>  them.
>
>  This looks ready for 'next'.  Any comments?

Looks good to me, although I would say that wouldn't I? :)

[PATCH 2/7] Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL

[Johannes Schindelin]

Nowadays, there are practical collision attacks on the SHA-1 algorithm.
For that reason, Git integrated code that detects attempts to sneak in
objects using those known attack vectors and enabled it by default.

The collision detection is not for free, though: when using the SHA1DC
code, calculating the SHA-1 takes substantially longer than using
OpenSSL's (in some case hardware-accelerated) SHA1-routines, and this
happens even when switching off the collision detection in SHA1DC's code.

Therefore, it makes sense to limit the use of the collision-detecting
SHA1DC to the cases where objects are introduced from any outside source,
and use the fast OpenSSL code instead when working on implicitly trusted
data (such as when the user calls `git add`).

This patch introduces the DC_AND_OPENSSL_SHA1 Makefile knob to compile
with both SHA1DC and OpenSSL's SHA-1 routines, defaulting to SHA1DC. A
later patch will add the ability to switch between them.

Signed-off-by: Johannes Schindelin 
---
 Makefile  | 12 
 hash.h|  2 +-
 sha1dc/sha1.c | 21 +
 sha1dc/sha1.h | 16 
 4 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 9f8b35ad41f..3e181d2f0e2 100644
--- a/Makefile
+++ b/Makefile
@@ -147,6 +147,11 @@ all::
 # Define OPENSSL_SHA1 environment variable when running make to link
 # with the SHA1 routine from openssl library.
 #
+# Define DC_AND_OPENSSL_SHA1 environment variable when running make to use
+# the collision-detecting sha1 algorithm by default, configurable via the
+# core.enableSHA1DC setting, falling back to OpenSSL's faster algorithm when
+# the collision detection is disabled.
+#
 # Define SHA1_MAX_BLOCK_SIZE to limit the amount of data that will be hashed
 # in one call to the platform's SHA1_Update(). e.g. APPLE_COMMON_CRYPTO
 # wants 'SHA1_MAX_BLOCK_SIZE=1024L*1024L*1024L' defined.
@@ -1391,6 +1396,12 @@ ifdef APPLE_COMMON_CRYPTO
SHA1_MAX_BLOCK_SIZE = 1024L*1024L*1024L
 endif
 
+ifdef DC_AND_OPENSSL_SHA1
+   EXTLIBS += $(LIB_4_CRYPTO)
+   LIB_OBJS += sha1dc/sha1.o
+   LIB_OBJS += sha1dc/ubc_check.o
+   BASIC_CFLAGS += -DSHA1_DC_AND_OPENSSL
+else
 ifdef OPENSSL_SHA1
EXTLIBS += $(LIB_4_CRYPTO)
BASIC_CFLAGS += -DSHA1_OPENSSL
@@ -1415,6 +1426,7 @@ endif
 endif
 endif
 endif
+endif
 
 ifdef SHA1_MAX_BLOCK_SIZE
LIB_OBJS += compat/sha1-chunked.o
diff --git a/hash.h b/hash.h
index a11fc9233fc..3a09343270d 100644
--- a/hash.h
+++ b/hash.h
@@ -7,7 +7,7 @@
 #include 
 #elif defined(SHA1_OPENSSL)
 #include 
-#elif defined(SHA1_DC)
+#elif defined(SHA1_DC) || defined(SHA1_DC_AND_OPENSSL)
 #include "sha1dc/sha1.h"
 #else /* SHA1_BLK */
 #include "block-sha1/sha1.h"
diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index d99db4f2e1b..e6bcf0ffa86 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -1806,3 +1806,24 @@ void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *vdata, 
unsigned long len)
}
SHA1DCUpdate(ctx, data, len);
 }
+
+#ifdef SHA1_DC_AND_OPENSSL
+void (*SHA1_Init_func)(SHA_CTX_union *ctx) = (void *)SHA1DCInit;
+void (*SHA1_Update_func)(SHA_CTX_union *ctx, const void *pointer, size_t size) 
=
+   (void *)git_SHA1DCUpdate;
+int (*SHA1_Final_func)(unsigned char sha1[20], SHA_CTX_union *ctx) =
+   (void *)git_SHA1DCFinal;
+
+void toggle_sha1dc(int enable)
+{
+   if (enable) {
+   SHA1_Init_func = (void *)SHA1DCInit;
+   SHA1_Update_func = (void *)git_SHA1DCUpdate;
+   SHA1_Final_func = (void *)git_SHA1DCFinal;
+   } else {
+   SHA1_Init_func = (void *)SHA1_Init;
+   SHA1_Update_func = (void *)SHA1_Update;
+   SHA1_Final_func = (void *)SHA1_Final;
+   }
+}
+#endif
diff --git a/sha1dc/sha1.h b/sha1dc/sha1.h
index bd8bd928fb3..243c2fe0b6b 100644
--- a/sha1dc/sha1.h
+++ b/sha1dc/sha1.h
@@ -110,10 +110,26 @@ void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);
  */
 void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);
 
+#ifdef SHA1_DC_AND_OPENSSL
+extern void toggle_sha1dc(int enable);
+
+typedef union {
+   SHA1_CTX dc;
+   SHA_CTX openssl;
+} SHA_CTX_union;
+extern void (*SHA1_Init_func)(SHA_CTX_union *ctx);
+extern void (*SHA1_Update_func)(SHA_CTX_union *ctx, const void *data, size_t 
len);
+extern int (*SHA1_Final_func)(unsigned char sha1[20], SHA_CTX_union *ctx);
+#define platform_SHA_CTX SHA_CTX_union
+#define platform_SHA1_Init SHA1_Init_func
+#define platform_SHA1_Update SHA1_Update_func
+#define platform_SHA1_Final SHA1_Final_func
+#else
 #define platform_SHA_CTX SHA1_CTX
 #define platform_SHA1_Init SHA1DCInit
 #define platform_SHA1_Update git_SHA1DCUpdate
 #define platform_SHA1_Final git_SHA1DCFinal
+#endif
 
 #if defined(__cplusplus)
 }
-- 
2.12.1.windows.1

[PATCH 1/7] sha1dc: safeguard against outside definitions of BIGENDIAN

[Johannes Schindelin]

In sha1dc/sha1.c, we #define BIGENDIAN under certain circumstances, and
obviously leave the door open for scenarios where our conditions do not
catch and that constant is #defined elsewhere.

However, we did not expect that anybody would possibly #define BIGENDIAN
to 0, indicating that the current platform is *not* big endian.

This is not just a theoretical consideration: On Windows, the winsock2.h
header file (which is used to allow Git to communicate via network) does
indeed do this.

Let's test for that circumstance, too, and byte-swap as intended in that
case.

This fixes a massive breakage on Windows where current `pu` (having
switched on DC_SHA1 by default) breaks pretty much every single test
case.

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

diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 6dd0da36084..d99db4f2e1b 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -35,7 +35,7 @@
 
 #define sha1_mix(W, t)  (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 
16], 1))
 
-#if defined(BIGENDIAN)
+#if defined(BIGENDIAN) && BIGENDIAN != 0
#define sha1_load(m, t, temp)  { temp = m[t]; }
 #else
#define sha1_load(m, t, temp)  { temp = m[t]; sha1_bswap32(temp); }
-- 
2.12.1.windows.1

[PATCH 5/7] t0013: test DC_AND_OPENSSL_SHA1, too

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin 
---
 Makefile |  1 +
 t/helper/test-sha1.c | 10 ++
 t/t0013-sha1dc.sh| 10 ++
 3 files changed, 21 insertions(+)

diff --git a/Makefile b/Makefile
index 3e181d2f0e2..0b581357625 100644
--- a/Makefile
+++ b/Makefile
@@ -2251,6 +2251,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst 
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@+
@echo PAGER_ENV=\''$(subst ','\'',$(subst ','\'',$(PAGER_ENV)))'\' >>$@+
@echo DC_SHA1=\''$(subst ','\'',$(subst ','\'',$(DC_SHA1)))'\' >>$@+
+   @echo DC_AND_OPENSSL_SHA1=\''$(subst ','\'',$(subst 
','\'',$(DC_AND_OPENSSL_SHA1)))'\' >>$@+
 ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst 
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@+
 endif
diff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c
index a1c13f54eca..27ce8869e51 100644
--- a/t/helper/test-sha1.c
+++ b/t/helper/test-sha1.c
@@ -8,6 +8,16 @@ int cmd_main(int ac, const char **av)
int binary = 0;
char *buffer;
 
+   if (ac > 1 && !strcmp(av[1], "--disable-sha1dc")) {
+#ifdef SHA1_DC_AND_OPENSSL
+   toggle_sha1dc(0);
+#else
+   die("Not compiled with DC_AND_OPENSSL_SHA1");
+#endif
+   ac--;
+   av++;
+   }
+
if (ac == 2) {
if (!strcmp(av[1], "-b"))
binary = 1;
diff --git a/t/t0013-sha1dc.sh b/t/t0013-sha1dc.sh
index 435a96d6108..2b529b31b4e 100755
--- a/t/t0013-sha1dc.sh
+++ b/t/t0013-sha1dc.sh
@@ -5,6 +5,10 @@ test_description='test sha1 collision detection'
 TEST_DATA="$TEST_DIRECTORY/t0013"
 
 test -z "$DC_SHA1" || test_set_prereq DC_SHA1
+test -z "$DC_AND_OPENSSL_SHA1" || {
+   test_set_prereq DC_AND_OPENSSL_SHA1
+   test_set_prereq DC_SHA1
+}
 
 test_expect_success DC_SHA1 'test-sha1 detects shattered pdf' '
test_must_fail test-sha1 <"$TEST_DATA/shattered-1.pdf" 2>err &&
@@ -12,4 +16,10 @@ test_expect_success DC_SHA1 'test-sha1 detects shattered 
pdf' '
grep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err
 '
 
+test_expect_success DC_AND_OPENSSL_SHA1 'sha1dc can be turned off' '
+   test-sha1 --disable-sha1dc <"$TEST_DATA/shattered-1.pdf" 2>err &&
+   ! test_i18ngrep collision err &&
+   ! grep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err
+'
+
 test_done
-- 
2.12.1.windows.1

[PATCH 3/7] config: add the core.enablesha1dc setting

[Johannes Schindelin]

When compiled with DC_AND_OPENSSL_SHA1, this new config setting allows to
switch from the collision-detecting SHA-1 routines (SHA1DC) to the
noticeably faster OpenSSL ones.

The default is still to detect collisions.

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

diff --git a/config.c b/config.c
index 1a4d85537b3..f94e2963e57 100644
--- a/config.c
+++ b/config.c
@@ -1205,6 +1205,14 @@ static int git_default_core_config(const char *var, 
const char *value)
return 0;
}
 
+   if (!strcmp(var, "core.enablesha1dc")) {
+#ifdef DC_AND_OPENSSL_SHA1
+   toggle_sha1dc(git_config_bool(var, value));
+#else
+   warning("Ignoring core.enablesha1dc='%s'", value);
+#endif
+   }
+
/* Add other config variables here and to Documentation/config.txt. */
return 0;
 }
-- 
2.12.1.windows.1

[PATCH 4/7] t0013: do not skip the entire file wholesale without DC_SHA1

[Johannes Schindelin]

So far, there is only one test case in that script, and that case indeed
requires that the code was compiled with with the DC_SHA1 flag.

However, we are about to add another test case to verify that the
DC_AND_OPENSSL_SHA1 flag works correctly, too.

So let's refactor the code a little.

Signed-off-by: Johannes Schindelin 
---
 t/t0013-sha1dc.sh | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/t/t0013-sha1dc.sh b/t/t0013-sha1dc.sh
index 6d655cb161b..435a96d6108 100755
--- a/t/t0013-sha1dc.sh
+++ b/t/t0013-sha1dc.sh
@@ -4,13 +4,9 @@ test_description='test sha1 collision detection'
 . ./test-lib.sh
 TEST_DATA="$TEST_DIRECTORY/t0013"
 
-if test -z "$DC_SHA1"
-then
-   skip_all='skipping sha1 collision tests, DC_SHA1 not set'
-   test_done
-fi
+test -z "$DC_SHA1" || test_set_prereq DC_SHA1
 
-test_expect_success 'test-sha1 detects shattered pdf' '
+test_expect_success DC_SHA1 'test-sha1 detects shattered pdf' '
test_must_fail test-sha1 <"$TEST_DATA/shattered-1.pdf" 2>err &&
test_i18ngrep collision err &&
grep 38762cf7f55934b34d179ae6a4c80cadccbb7f0a err
-- 
2.12.1.windows.1

[PATCH 0/7] PREVIEW: Introduce DC_AND_OPENSSL_SHA1 make flag

[Johannes Schindelin]

As I pointed out several times in the past, the performance hit of
enabling SHA1DC globally is not acceptable. This patch series not only
demonstrates that clearly in the perf test it adds (it is the last patch
in the current series, and its commit message has some numbers), it also
shows an early glimpse of what I will be proposing to fix the situation.

Obviously, this patch series is not complete yet:

- the first patch is an obvious bug fix that I sent out separately, but
  it is needed to unbreak almost all tests on Windows.

- the most important part will be the patch turning core.enableSHA1DC
  into a tristate: "externalOnly" or "smart" or "auto" or something
  indicating that it switches on collision detection only for commands
  that accept objects from an outside source into the local repository,
  such as fetch, fast-import, etc


Johannes Schindelin (7):
  sha1dc: safeguard against outside definitions of BIGENDIAN
  Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL
  config: add the core.enablesha1dc setting
  t0013: do not skip the entire file wholesale without DC_SHA1
  t0013: test DC_AND_OPENSSL_SHA1, too
  mingw: enable DC_AND_OPENSSL_SHA1 by default
  p0013: new test to compare SHA1DC vs OpenSSL

 Makefile   | 13 +
 config.c   |  8 
 config.mak.uname   |  1 +
 hash.h |  2 +-
 sha1dc/sha1.c  | 23 ++-
 sha1dc/sha1.h  | 16 
 t/helper/test-sha1.c   | 10 ++
 t/perf/p0013-sha1dc.sh | 23 +++
 t/t0013-sha1dc.sh  | 18 --
 9 files changed, 106 insertions(+), 8 deletions(-)
 create mode 100644 t/perf/p0013-sha1dc.sh


base-commit: 063fe858b89ef8ee27965115fd6b1ed12e42e793
Published-As: https://github.com/dscho/git/releases/tag/sha1dc-knob-v1
Fetch-It-Via: git fetch https://github.com/dscho/git sha1dc-knob-v1

-- 
2.12.1.windows.1

[PATCH/RFC] parse-options: add facility to make options configurable

[Ævar Arnfjörð Bjarmason]

Add a nascent WIP facility to specify via the options parsing that
we'd e.g. like to grab the --status option for commit.status from the
commit.status config key.

This is all very proof-of-concept, and uses the ugly hack of s/const
// for the options struct because I'm now keeping state in it, as
noted in one of the TODO comments that should be moved.

But even so this works, passes all tests, gets rid of 3 lines in
commit.c, and has the promise of getting rid of a *lot* more manual
option parsing in favor of declaring config keys via OPT_*
everywhere. See my

for more details.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---

On Sun, Mar 19, 2017 at 2:43 PM, Ævar Arnfjörð Bjarmason  
wrote:
> I don't know if this is what Duy has in mind, but the facility I've
> described is  purely an internal code reorganization issue. I.e. us
> not having to write custom code for each bultin every time we want to
> take an option from the command line || config.

Here's an implementation of this I hacked up this evening. This is
very WIP as noted in the commit message / TODO comments, but it works!
I thought I'd send it to the list for comments on the general approach
before taking it much further.

 builtin/commit.c   |  7 ++-
 parse-options-cb.c | 21 +
 parse-options.c| 40 
 parse-options.h| 16 +---
 4 files changed, 72 insertions(+), 12 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc513..a7c9e4128f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1500,10 +1500,6 @@ static int git_commit_config(const char *k, const char 
*v, void *cb)
 
if (!strcmp(k, "commit.template"))
return git_config_pathname(_file, k, v);
-   if (!strcmp(k, "commit.status")) {
-   include_status = git_config_bool(k, v);
-   return 0;
-   }
if (!strcmp(k, "commit.cleanup"))
return git_config_string(_arg, k, v);
if (!strcmp(k, "commit.gpgsign")) {
@@ -1596,7 +1592,8 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
OPT_FILENAME('t', "template", _file, N_("use specified 
template file")),
OPT_BOOL('e', "edit", _flag, N_("force edit of commit")),
OPT_STRING(0, "cleanup", _arg, N_("default"), N_("how 
to strip spaces and #comments from message")),
-   OPT_BOOL(0, "status", _status, N_("include status in 
commit message template")),
+   OPT_BOOL_C(0, "status", _status, N_("include status in 
commit message template"),
+  "commit.status", parse_opt_confkey_bool),
{ OPTION_STRING, 'S', "gpg-sign", _commit, N_("key-id"),
  N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" 
},
/* end commit message options */
diff --git a/parse-options-cb.c b/parse-options-cb.c
index b7d8f7dcb2..2383d9bbe0 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -236,3 +236,24 @@ int parse_opt_passthru_argv(const struct option *opt, 
const char *arg, int unset
 
return 0;
 }
+
+/* Does it suck that I have to use the cache interface to the config
+ * here? Should we somehow unroll this whole thing so
+ * parse_options_step loops over the config values, and maybe
+ * populates opt->conf_key (which we'd need to add) for all the
+ * options that need it?
+ *
+ * I.e. should we make this more complex because this one-shot
+ * interface is expensive, or is it just fine?
+*/ 
+
+int parse_opt_confkey_bool(const struct option *opt, const char *arg, int 
unset) {
+   const char *value;
+
+   if (git_config_get_value(opt->conf_key, ))
+   return 0;
+
+   *(int *)opt->value = git_config_bool(opt->conf_key, value);
+
+   return 0;
+}
diff --git a/parse-options.c b/parse-options.c
index 4fbe924a5d..f9aba088d8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -235,12 +235,13 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, 
const struct option *optio
 }
 
 static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
-  const struct option *options)
+  struct option *options)
 {
const struct option *all_opts = options;
const char *arg_end = strchrnul(arg, '=');
const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
int abbrev_flags = 0, ambiguous_flags = 0;
+   int ret;
 
for (; options->type != OPTION_END; options++) {
const char *rest, *long_name = options->long_name;
@@ -313,7 +314,17 @@ static int parse_long_opt(struct parse_opt_ctx_t *p, const 
char *arg,
continue;
p->opt = rest + 1;
}
-   return get_value(p, options, all_opts, flags ^ 

Re: [PATCH 6/7] short status: improve reporting for submodule changes

[Jonathan Nieder]

Stefan Beller wrote:

> +++ b/wt-status.c
> @@ -431,10 +431,19 @@ static void wt_status_collect_changed_cb(struct 
> diff_queue_struct *q,
>   }
>   if (!d->worktree_status)
>   d->worktree_status = p->status;
> - d->dirty_submodule = p->two->dirty_submodule;
> - if (S_ISGITLINK(p->two->mode))
> + if (S_ISGITLINK(p->two->mode)) {
> + d->dirty_submodule = p->two->dirty_submodule;

This is to simplify because dirty_submodule is just going to stay as 0
in the !S_ISGITLINK(p->two->mode) case, right?

>   d->new_submodule_commits = !!oidcmp(>one->oid,
>   >two->oid);
> + if (s->status_format == STATUS_FORMAT_SHORT) {
> + if (d->new_submodule_commits)
> + d->worktree_status = 'M';
> + else if (d->dirty_submodule & 
> DIRTY_SUBMODULE_MODIFIED)
> + d->worktree_status = 'm';
> + else if (d->dirty_submodule & 
> DIRTY_SUBMODULE_UNTRACKED)
> + d->worktree_status = '?';
> + }
> + }

Makes sense.

This patch also goes past the right margin.  Maybe this code to compute
d->worktree_status could be its own function?

With or without such a change,
Reviewed-by: Jonathan Nieder 

diff --git i/wt-status.c w/wt-status.c
index 9909fd0e57..0375484962 100644
--- i/wt-status.c
+++ w/wt-status.c
@@ -407,6 +407,16 @@ static void wt_longstatus_print_change_data(struct 
wt_status *s,
strbuf_release();
 }
 
+static char short_submodule_status(struct wt_status_change_data *d) {
+   if (d->new_submodule_commits)
+   return 'M';
+   if (d->dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
+   return 'm';
+   if (d->dirty_submodule & DIRTY_SUBMODULE_UNTRACKED)
+   return '?';
+   return d->worktree_status;
+}
+
 static void wt_status_collect_changed_cb(struct diff_queue_struct *q,
 struct diff_options *options,
 void *data)
@@ -435,14 +445,8 @@ static void wt_status_collect_changed_cb(struct 
diff_queue_struct *q,
d->dirty_submodule = p->two->dirty_submodule;
d->new_submodule_commits = !!oidcmp(>one->oid,
>two->oid);
-   if (s->status_format == STATUS_FORMAT_SHORT) {
-   if (d->new_submodule_commits)
-   d->worktree_status = 'M';
-   else if (d->dirty_submodule & 
DIRTY_SUBMODULE_MODIFIED)
-   d->worktree_status = 'm';
-   else if (d->dirty_submodule & 
DIRTY_SUBMODULE_UNTRACKED)
-   d->worktree_status = '?';
-   }
+   if (s->status_format == STATUS_FORMAT_SHORT)
+   d->worktree_status = short_submodule_status(d);
}
 
switch (p->status) {

Re: [PATCH v4 1/2] diff --no-index: optionally follow symlinks

[Junio C Hamano]

Dennis Kaarsemaker  writes:

> @@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
>  #endif
>   else if (path == file_from_standard_input)
>   *mode = create_ce_mode(0666);
> - else if (lstat(path, ))
> + else if (dereference ? stat(path, ) : lstat(path, ))
>   return error("Could not access '%s'", path);
>   else
>   *mode = st.st_mode;

This part makes sense---when the caller tells us to stat() we
stat(), otherwise, we lstat().

> diff --git a/diff.c b/diff.c
> index be11e4ef2b..2afecfb939 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
> unsigned int flags)
>   s->size = xsize_t(st.st_size);
>   if (!s->size)
>   goto empty;
> - if (S_ISLNK(st.st_mode)) {
> + if (S_ISLNK(s->mode)) {

This change is conceptually wrong.  s->mode (often) comes from the
index but in this codepath, after finding that s->oid is not valid
or we want to read from the working tree instead (several lines
before this part), we are committed to read from the working tree
and check things with st.st_* fields, not s->mode, when we decide
what to do with the thing we find on the filesystem, no?

> @@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, 
> unsigned int flags)
>   s->should_free = 1;
>   return 0;
>   }
> + if (S_ISLNK(st.st_mode)) {
> + stat(s->path, );
> + s->size = xsize_t(st.st_size);
> + }
>   if (size_only)
>   return 0;
>   if ((flags & CHECK_BINARY) &&

I suspect that this would conflict with a recent topic.  

But more importantly, this inserted code feels doubly wrong.

 - what allows us to unconditionally do "ah, symbolic link on the
   disk--find the target of the link, not the symbolic link itself"?
   We do not seem to be checking '--dereference' around here.

 - does this code do a reasonable thing when the path is a symbolic
   link that points at a directory?  what does it mean to grab
   st.st_size for such a thing (and then go on to open() and xmmap()
   it)?

Puzzled.

Thanks.


   

[PATCH] sha1dc: safeguard against outside definitions of BIGENDIAN

[Johannes Schindelin]

In sha1dc/sha1.c, we #define BIGENDIAN under certain circumstances, and
obviously leave the door open for scenarios where our conditions do not
catch and that constant is #defined elsewhere.

However, we did not expect that anybody would possibly #define BIGENDIAN
to 0, indicating that the current platform is *not* big endian.

This is not just a theoretical consideration: On Windows, the winsock2.h
header file (which is used to allow Git to communicate via network) does
indeed do this.

Let's test for that circumstance, too, and byte-swap as intended in that
case.

This fixes a massive breakage on Windows where current `pu` (having
switched on DC_SHA1 by default) breaks pretty much every single test
case.

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/sha1dc-bigendian=0-v1
Fetch-It-Via: git fetch https://github.com/dscho/git sha1dc-bigendian=0-v1

Obviously, this patch is based on `next`.

 sha1dc/sha1.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c
index 6dd0da36084..d99db4f2e1b 100644
--- a/sha1dc/sha1.c
+++ b/sha1dc/sha1.c
@@ -35,7 +35,7 @@
 
 #define sha1_mix(W, t)  (rotate_left(W[t - 3] ^ W[t - 8] ^ W[t - 14] ^ W[t - 
16], 1))
 
-#if defined(BIGENDIAN)
+#if defined(BIGENDIAN) && BIGENDIAN != 0
#define sha1_load(m, t, temp)  { temp = m[t]; }
 #else
#define sha1_load(m, t, temp)  { temp = m[t]; sha1_bswap32(temp); }

base-commit: c21884356fab0bc6bc5fa6abcadbda27a112a76c
-- 
2.12.1.windows.1

Re: [PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

> By having a stricter check in the superproject we catch errors earlier,
> instead of spawning a child process to tell us.
> 
> Signed-off-by: Stefan Beller 
> Reviewed-by: Jonathan Nieder 

Yep. :)

Re: [PATCH 4/7] submodule.c: port is_submodule_modified to use porcelain 2

[Jonathan Nieder]

Stefan Beller wrote:

[...]
> --- a/submodule.c
> +++ b/submodule.c
[...]
> @@ -1070,11 +1070,12 @@ unsigned is_submodule_modified(const char *path, int 
> ignore_untracked)
[...]
>   while (strbuf_getwholeline(, fp, '\n') != EOF) {
> - if ((buf.buf[0] == '?') && (buf.buf[1] == '?'))
> + /* regular untracked files */
> + if (buf.buf[0] == '?')
>   dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
>   else
>   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;

At first I didn't believe it could be so simple. :)

Unlike ordinary files that use 1, 2, or u, ignored files use '!'.
You're not passing --ignored so you don't have to worry about them.

Reviewed-by: Jonathan Nieder 

Very nice.

Re: [PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline

[Jonathan Nieder]

Stefan Beller wrote:

> Instead of implementing line reading yet again, make use of our beautiful
> library function to read one line.  By using strbuf_getwholeline instead
> of strbuf_read, we avoid having to allocate memory for the entire child
> process output at once.  That is, we limit maximum memory usage.

It also overlaps work a little better.

> Once we know all information that we care about, we can terminate
> the child early. In that case we do not care about its exit code as well.

Should this say something about SIGPIPE?

[...]
> +++ b/submodule.c
[...]
> @@ -1072,28 +1072,27 @@ unsigned is_submodule_modified(const char *path, int 
> ignore_untracked)
[...]
> + /*
> +  * We're not interested in any further information from
> +  * the child any more, no output nor its exit code.
> +  */

language nit: s/, no output/: neither output/

[...]
> - if (finish_command())
> + if (finish_command() && !ignore_cp_exit_code)

finish_command complains if the child dies of SIGTERM:

error: status died of signal 15

wait_or_whine(cp->pid, cp->argv[0], 1) doesn't do that but is meant for
signal handling.  Maybe we should rely on SIGPIPE instead (which
wait_or_whine always silences) and avoid the kill() call.

Can there be a test for this case (i.e. having lots of untracked files
in the submodule so the child process fills its pipe buffer and has to
exit early)?

Thanks,
Jonathan

Re: [PATCH 2/7] submodule.c: factor out early loop termination in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

> --- a/submodule.c
> +++ b/submodule.c
> @@ -1075,16 +1075,15 @@ unsigned is_submodule_modified(const char *path, int 
> ignore_untracked)
>   len = strbuf_read(, cp.out, 1024);
>   line = buf.buf;
>   while (len > 2) {
> - if ((line[0] == '?') && (line[1] == '?')) {
> + if ((line[0] == '?') && (line[1] == '?'))
>   dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
> - if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
> - break;
> - } else {
> + else
>   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
> - if (ignore_untracked ||
> - (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
> - break;
> - }
> +
> + if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
> + ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || 
> ignore_untracked))

nit: long line

Reviewed-by: Jonathan Nieder 

diff --git i/submodule.c w/submodule.c
index aba89661a7..8934d97359 100644
--- i/submodule.c
+++ w/submodule.c
@@ -1087,8 +1087,10 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 
if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
-   ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || 
ignore_untracked))
+   (ignore_untracked
+|| (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))) {
break;
+   }
 
next_line = strchr(line, '\n');
if (!next_line)

Re: [PATCH 1/7] submodule.c: use argv_array in is_submodule_modified

[Jonathan Nieder]

Stefan Beller wrote:

> struct argv_array is easier to use and maintain

Missing '.' at end of sentence.

> Signed-off-by: Stefan Beller 
> ---
>  submodule.c | 10 ++
>  1 file changed, 2 insertions(+), 8 deletions(-)

With or without that tweak, I still like this as much as last time. :)
Reviewed-by: Jonathan Nieder 

Thanks.

[PATCH v2] send-email: Net::SMTP::SSL is obsolete, use only when necessary

[Dennis Kaarsemaker]

Net::SMTP itself can do the necessary SSL and STARTTLS bits just fine
since version 1.28, and Net::SMTP::SSL is now deprecated. Since 1.28
isn't that old yet, keep the old code in place and use it when
necessary.

While we're in the area, mark some messages for translation that were
not yet marked as such.

Signed-off-by: Dennis Kaarsemaker 
---
 git-send-email.perl | 54 ++---
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index eea0a517f7..0d90439d9a 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1353,10 +1353,12 @@ EOF
die __("The required SMTP server is not properly 
defined.")
}
 
+   require Net::SMTP;
+   my $use_net_smtp_ssl = version->parse($Net::SMTP::VERSION) < 
version->parse("1.28");
+   $smtp_domain ||= maildomain();
+
if ($smtp_encryption eq 'ssl') {
$smtp_server_port ||= 465; # ssmtp
-   require Net::SMTP::SSL;
-   $smtp_domain ||= maildomain();
require IO::Socket::SSL;
 
# Suppress "variable accessed once" warning.
@@ -1368,34 +1370,48 @@ EOF
# Net::SMTP::SSL->new() does not forward any SSL options
IO::Socket::SSL::set_client_defaults(
ssl_verify_params());
-   $smtp ||= Net::SMTP::SSL->new($smtp_server,
- Hello => $smtp_domain,
- Port => $smtp_server_port,
- Debug => $debug_net_smtp);
+
+   if ($use_net_smtp_ssl) {
+   require Net::SMTP::SSL;
+   $smtp ||= Net::SMTP::SSL->new($smtp_server,
+ Hello => 
$smtp_domain,
+ Port => 
$smtp_server_port,
+ Debug => 
$debug_net_smtp);
+   }
+   else {
+   $smtp ||= Net::SMTP->new($smtp_server,
+Hello => $smtp_domain,
+Port => 
$smtp_server_port,
+Debug => 
$debug_net_smtp,
+SSL => 1);
+   }
}
else {
-   require Net::SMTP;
-   $smtp_domain ||= maildomain();
$smtp_server_port ||= 25;
$smtp ||= Net::SMTP->new($smtp_server,
 Hello => $smtp_domain,
 Debug => $debug_net_smtp,
 Port => $smtp_server_port);
if ($smtp_encryption eq 'tls' && $smtp) {
-   require Net::SMTP::SSL;
-   $smtp->command('STARTTLS');
-   $smtp->response();
-   if ($smtp->code == 220) {
+   if ($use_net_smtp_ssl) {
+   $smtp->command('STARTTLS');
+   $smtp->response();
+   if ($smtp->code != 220) {
+   die sprintf(__("Server does not 
support STARTTLS! %s"), $smtp->message);
+   }
+   require Net::SMTP::SSL;
$smtp = Net::SMTP::SSL->start_SSL($smtp,
  
ssl_verify_params())
-   or die "STARTTLS failed! 
".IO::Socket::SSL::errstr();
-   $smtp_encryption = '';
-   # Send EHLO again to receive fresh
-   # supported commands
-   $smtp->hello($smtp_domain);
-   } else {
-   die sprintf(__("Server does not support 
STARTTLS! %s"), $smtp->message);
+   or die sprintf(__("STARTTLS 
failed! %s"), IO::Socket::SSL::errstr());
+   }
+   else {
+   $smtp->starttls(ssl_verify_params())
+ 

[PATCH v4 0/2] diff --no-index: support symlinks and pipes

[Dennis Kaarsemaker]

git diff <(command1) <(command2) is less useful than it could be, all it 
outputs is:

diff --git a/dev/fd/63 b/dev/fd/62
index 9e6542b297..9f7b2c291b 12
--- a/dev/fd/63
+++ b/dev/fd/62
@@ -1 +1 @@
-pipe:[464811685]
\ No newline at end of file
+pipe:[464811687]
\ No newline at end of file

Normal diff provides arguably better output: the diff of the output of the
commands. This series makes it possible for git diff --no-index to follow
symlinks and read from pipes, mimicking the behaviour of normal diff.

v1: http://public-inbox.org/git/2016201958.2175-1-den...@kaarsemaker.net/
v2: http://public-inbox.org/git/20170113102021.6054-1-den...@kaarsemaker.net/
v3: http://public-inbox.org/git/20170318210038.22638-1-den...@kaarsemaker.net/

Changes since v3:
Using the --dereference option without being in explicit or implicit no-index
mode is no longer silently ignored, but an error. A test has been added for
this behaviour.

Dennis Kaarsemaker (2):
  diff --no-index: optionally follow symlinks
  diff --no-index: support reading from pipes

 Documentation/diff-options.txt |  9 +++
 builtin/diff.c |  2 ++
 diff-no-index.c| 16 ++---
 diff.c | 30 +++
 diff.h |  2 +-
 t/t4011-diff-symlink.sh|  6 +
 t/t4053-diff-no-index.sh   | 54 ++
 t/test-lib.sh  |  4 
 8 files changed, 115 insertions(+), 8 deletions(-)

-- 
2.12.0-488-gd3584ba

[PATCH v4 1/2] diff --no-index: optionally follow symlinks

[Dennis Kaarsemaker]

Git's diff machinery does not follow symlinks, which makes sense as git
itself also does not, but stores the symlink destination.

In --no-index mode however, it is useful for diff to be able to follow
symlinks, matching the behaviour of ordinary diff. A new --dereference
(name copied from diff) option has been added to enable this behaviour.
--no-dereference can be used to disable it again.

Signed-off-by: Dennis Kaarsemaker 
---
 Documentation/diff-options.txt |  9 +
 builtin/diff.c |  2 ++
 diff-no-index.c|  7 ---
 diff.c | 12 ++--
 diff.h |  2 +-
 t/t4011-diff-symlink.sh|  6 ++
 t/t4053-diff-no-index.sh   | 44 ++
 7 files changed, 76 insertions(+), 6 deletions(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 2d77a19626..5a9d58b701 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -216,6 +216,15 @@ any of those replacements occurred.
commit range.  Defaults to `diff.submodule` or the 'short' format
if the config option is unset.
 
+ifdef::git-diff[]
+--dereference::
+--no-dereference::
+   Normally, "git diff --no-index" will compare symlinks by comparing what
+   they point to. The `--dereference` option will make it compare the 
content
+   of the linked files. The `--no-dereference` option disables an earlier
+   `--dereference`.
+endif::git-diff[]
+
 --color[=]::
Show colored diff.
`--color` (i.e. without '=') is the same as `--color=always`.
diff --git a/builtin/diff.c b/builtin/diff.c
index 7f91f6d226..09e646060e 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -360,6 +360,8 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
if (nongit)
die(_("Not a git repository"));
argc = setup_revisions(argc, argv, , NULL);
+   if (DIFF_OPT_TST(, DEREFERENCE))
+   die(_("--dereference can only be used together with 
--no-index"));
if (!rev.diffopt.output_format) {
rev.diffopt.output_format = DIFF_FORMAT_PATCH;
diff_setup_done();
diff --git a/diff-no-index.c b/diff-no-index.c
index f420786039..fe48f32ddd 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -40,7 +40,7 @@ static int read_directory_contents(const char *path, struct 
string_list *list)
  */
 static const char file_from_standard_input[] = "-";
 
-static int get_mode(const char *path, int *mode)
+static int get_mode(const char *path, int *mode, int dereference)
 {
struct stat st;
 
@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
 #endif
else if (path == file_from_standard_input)
*mode = create_ce_mode(0666);
-   else if (lstat(path, ))
+   else if (dereference ? stat(path, ) : lstat(path, ))
return error("Could not access '%s'", path);
else
*mode = st.st_mode;
@@ -93,7 +93,8 @@ static int queue_diff(struct diff_options *o,
 {
int mode1 = 0, mode2 = 0;
 
-   if (get_mode(name1, ) || get_mode(name2, ))
+   if (get_mode(name1, , DIFF_OPT_TST(o, DEREFERENCE)) ||
+   get_mode(name2, , DIFF_OPT_TST(o, DEREFERENCE)))
return -1;
 
if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
diff --git a/diff.c b/diff.c
index be11e4ef2b..2afecfb939 100644
--- a/diff.c
+++ b/diff.c
@@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->size = xsize_t(st.st_size);
if (!s->size)
goto empty;
-   if (S_ISLNK(st.st_mode)) {
+   if (S_ISLNK(s->mode)) {
struct strbuf sb = STRBUF_INIT;
 
if (strbuf_readlink(, s->path, s->size))
@@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
s->should_free = 1;
return 0;
}
+   if (S_ISLNK(st.st_mode)) {
+   stat(s->path, );
+   s->size = xsize_t(st.st_size);
+   }
if (size_only)
return 0;
if ((flags & CHECK_BINARY) &&
@@ -3884,7 +3888,11 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--no-follow")) {
DIFF_OPT_CLR(options, FOLLOW_RENAMES);
DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
-   } else if (!strcmp(arg, "--color"))
+   } else if (!strcmp(arg, "--dereference"))
+   DIFF_OPT_SET(options, DEREFERENCE);
+   else if (!strcmp(arg, "--no-dereference"))
+   DIFF_OPT_CLR(options, DEREFERENCE);
+   else if (!strcmp(arg, "--color"))
options->use_color = 1;
else if 

[PATCH v4 2/2] diff --no-index: support reading from pipes

[Dennis Kaarsemaker]

diff <(command1) <(command2) provides useful output, let's make it
possible for git to do the same.

Signed-off-by: Dennis Kaarsemaker 
---
 diff-no-index.c  |  9 +
 diff.c   | 18 --
 t/t4053-diff-no-index.sh | 10 ++
 t/test-lib.sh|  4 
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index fe48f32ddd..1262a587e5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -83,6 +83,15 @@ static struct diff_filespec *noindex_filespec(const char 
*name, int mode)
name = "/dev/null";
s = alloc_filespec(name);
fill_filespec(s, null_sha1, 0, mode);
+   /*
+* In --no-index mode, we support reading from pipes. canon_mode, 
called by
+* fill_filespec, gets confused by this and thinks we now have 
subprojects.
+* To help the rest of the diff machinery along, we now override what
+* canon_mode says. This is done here instead of in canon_mode, because 
the
+* rest of git does not (and should not) support pipes.
+*/
+   if (S_ISFIFO(mode))
+   s->mode = S_IFREG | ce_permissions(mode);
if (name == file_from_standard_input)
populate_from_stdin(s);
return s;
diff --git a/diff.c b/diff.c
index 2afecfb939..4f74a54d74 100644
--- a/diff.c
+++ b/diff.c
@@ -2765,6 +2765,11 @@ static int diff_populate_gitlink(struct diff_filespec 
*s, int size_only)
return 0;
 }
 
+static int should_mmap_file_contents(struct stat *st)
+{
+   return S_ISREG(st->st_mode);
+}
+
 /*
  * While doing rename detection and pickaxe operation, we may need to
  * grab the data for the blob (or file) for our own in-core comparison.
@@ -2839,9 +2844,18 @@ int diff_populate_filespec(struct diff_filespec *s, 
unsigned int flags)
fd = open(s->path, O_RDONLY);
if (fd < 0)
goto err_empty;
-   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+   if (!should_mmap_file_contents()) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_read(, fd, 0);
+   s->size = sb.len;
+   s->data = strbuf_detach(, NULL);
+   s->should_free = 1;
+   }
+   else {
+   s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, 
fd, 0);
+   s->should_munmap = 1;
+   }
close(fd);
-   s->should_munmap = 1;
 
/*
 * Convert from working tree format to canonical git format
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 8c87bffb34..2d9b322315 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -171,4 +171,14 @@ test_expect_success SYMLINKS 'diff --no-index 
--no-dereference does not follow s
test_cmp expect actual
 '
 
+test_expect_success PROCESS_SUBSTITUTION 'diff --no-index works on fifos' '
+   cat >expect <<-EOF &&
+   @@ -1 +1 @@
+   -1
+   +2
+   EOF
+   test_expect_code 1 git diff --no-index --dereference <(echo 1) <(echo 
2) | tail -n +5 > actual &&
+   test_cmp expect actual
+'
+
 test_done
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11562bde10..78f3d24651 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1128,3 +1128,7 @@ build_option () {
 test_lazy_prereq LONG_IS_64BIT '
test 8 -le "$(build_option sizeof-long)"
 '
+
+test_lazy_prereq PROCESS_SUBSTITUTION '
+   eval "foo=<(echo test)" 2>/dev/null
+'
-- 
2.12.0-488-gd3584ba

What's cooking in git.git (Mar 2017, #10; Fri, 24)

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

The second maintenance release for v2.12.x series has just been
tagged.

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

--
[Graduated to "master"]

* dl/credential-cache-socket-in-xdg-cache (2017-03-17) 3 commits
  (merged to 'next' on 2017-03-20 at 9de71bcce8)
 + credential-cache: add tests for XDG functionality
 + credential-cache: use XDG_CACHE_HOME for socket
 + path.c: add xdg_cache_home

 The default location "~/.git-credential-cache/socket" for the
 socket used to communicate with the credential-cache daemon has
 been moved to "~/.cache/git/credential/socket".


* jk/execv-dashed-external (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at 62119fa314)
 + run-command: fix segfault when cleaning forked async process

 Fix for NO_PTHREADS build.


* jk/sha1dc (2017-03-17) 6 commits
  (merged to 'next' on 2017-03-20 at 3455b6c19f)
 + Makefile: make DC_SHA1 the default
 + t0013: add a basic sha1 collision detection test
 + Makefile: add DC_SHA1 knob
 + sha1dc: disable safe_hash feature
 + sha1dc: adjust header includes for git
 + sha1dc: add collision-detecting sha1 implementation

 The "detect attempt to create collisions" variant of SHA-1
 implementation by Marc Stevens (CWI) and Dan Shumow (Microsoft)
 has been integrated and made the default.


* js/regexec-buf (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at 7381595eb7)
 + pickaxe: fix segfault with '-S<...> --pickaxe-regex'

 Fix for potential segv introduced in v2.11.0 and later (also
 v2.10.2).


* rs/http-push-cleanup (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at fcf8d30bc0)
 + http-push: don't check return value of lookup_unknown_object()

 Code clean-up.


* rs/path-name-safety-cleanup (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at 78ea574469)
 + revision: remove declaration of path_name()

 Code clean-up.


* rs/shortlog-cleanup (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at a826dff5cf)
 + shortlog: don't set after_subject to an empty string

 Code clean-up.


* rs/update-hook-optim (2017-03-18) 1 commit
  (merged to 'next' on 2017-03-20 at f36ede55be)
 + receive-pack: simplify run_update_post_hook()

 Code clean-up.


* sg/test-with-stdin (2017-03-18) 2 commits
  (merged to 'next' on 2017-03-20 at a66fec5692)
 + tests: make the 'test_pause' helper work in non-verbose mode
 + tests: create an interactive gdb session with the 'debug' helper

 Teach the "debug" helper used in the test framework that allows a
 command to run under "gdb" to make the session interactive.

--
[New Topics]

* ab/test-readme-updates (2017-03-23) 4 commits
 - SQUASH???
 - t/README: clarify the test_have_prereq documentation
 - t/README: change "Inside  part" to "Inside the  part"
 - t/README: link to metacpan.org, not search.cpan.org

 Doc updates.

 Waiting for a reaction to SQUASH???


* jk/quote-env-path-list-component (2017-03-22) 1 commit
  (merged to 'next' on 2017-03-24 at 78843c4f9d)
 + t5615: fix a here-doc syntax error

 A test fix.

 Will merge to 'master'.


* js/rebase-i-reword-to-run-hooks (2017-03-23) 4 commits
 - SQUASH???
 - sequencer: allow the commit-msg hooks to run during a `reword`
 - sequencer: make commit options more extensible
 - t7504: document regression: reword no longer calls commit-msg

 A recent update to "rebase -i" stopped running hooks for the "git
 commit" command during "reword" action, which has been fixed.

 Waiting for a reaction to SQUASH???


* sb/push-options-via-transport (2017-03-22) 2 commits
  (merged to 'next' on 2017-03-24 at c5535bec3b)
 + remote-curl: allow push options
 + send-pack: send push options correctly in stateless-rpc case

 Recently we started passing the "--push-options" through the
 external remote helper interface; now the "smart HTTP" remote
 helper understands what to do with the passed information.

 Will merge to 'master'.


* sb/submodule-update-initial-runs-custom-script (2017-03-22) 1 commit
  (merged to 'next' on 2017-03-24 at 628200c3b1)
 + t7406: correct test case for submodule-update initial population

 A test fix.

 Will merge to 'master'.


* ab/branch-list-doc (2017-03-24) 2 commits
 - branch doc: update description for `--list`
 - branch doc: change `git branch ` to use ``

 Doc update.

 Will merge to 'next'.


* km/config-grammofix (2017-03-23) 1 commit
  (merged to 'next' on 2017-03-24 at 75ddbbc6f9)
 + doc/config: grammar fixes for core.{editor,commentChar}

 Doc update.

 Will merge to 'master'.


* sg/completion-ctags (2017-03-23) 3 commits
 - completion: 

A note from the maintainer

[Junio C Hamano]

Welcome to the Git development community.

This message is written by the maintainer and talks about how Git
project is managed, and how you can work with it.

* Mailing list and the community

The development is primarily done on the Git mailing list. Help
requests, feature proposals, bug reports and patches should be sent to
the list address .  You don't have to be
subscribed to send messages.  The convention on the list is to keep
everybody involved on Cc:, so it is unnecessary to say "Please Cc: me,
I am not subscribed".

Before sending patches, please read Documentation/SubmittingPatches
and Documentation/CodingGuidelines to familiarize yourself with the
project convention.

If you sent a patch and you did not hear any response from anybody for
several days, it could be that your patch was totally uninteresting,
but it also is possible that it was simply lost in the noise.  Please
do not hesitate to send a reminder message in such a case.  Messages
getting lost in the noise may be a sign that those who can evaluate
your patch don't have enough mental/time bandwidth to process them
right at the moment, and it often helps to wait until the list traffic
becomes calmer before sending such a reminder.

The list archive is available at a few public sites:

http://public-inbox.org/git/
http://marc.info/?l=git
http://www.spinics.net/lists/git/

For those who prefer to read it over NNTP:

nntp://news.public-inbox.org/inbox.comp.version-control.git
nntp://news.gmane.org/gmane.comp.version-control.git

are available.

When you point at a message in a mailing list archive, using its
message ID is often the most robust (if not very friendly) way to do
so, like this:


http://public-inbox.org/git/pine.lnx.4.58.0504150753440.7...@ppc970.osdl.org

Often these web interfaces accept the message ID with enclosing <>
stripped (like the above example to point at one of the most important
message in the Git list).

Some members of the development community can sometimes be found on
the #git and #git-devel IRC channels on Freenode.  Their logs are
available at:

http://colabti.org/irclogger/irclogger_log/git
http://colabti.org/irclogger/irclogger_log/git-devel

There is a volunteer-run newsletter to serve our community ("Git Rev
News" http://git.github.io/rev_news/rev_news.html).

Git is a member project of software freedom conservancy, a non-profit
organization (https://sfconservancy.org/).  To reach a committee of
liaisons to the conservancy, contact them at .


* Reporting bugs

When you think git does not behave as you expect, please do not stop
your bug report with just "git does not work".  "I used git in this
way, but it did not work" is not much better, neither is "I used git
in this way, and X happend, which is broken".  It often is that git is
correct to cause X happen in such a case, and it is your expectation
that is broken. People would not know what other result Y you expected
to see instead of X, if you left it unsaid.

Please remember to always state

 - what you wanted to achieve;

 - what you did (the version of git and the command sequence to reproduce
   the behavior);

 - what you saw happen (X above);

 - what you expected to see (Y above); and

 - how the last two are different.

See http://www.chiark.greenend.org.uk/~sgtatham/bugs.html for further
hints.

If you think you found a security-sensitive issue and want to disclose
it to us without announcing it to wider public, please contact us at
our security mailing list .  This is
a closed list that is limited to people who need to know early about
vulnerabilities, including:

  - people triaging and fixing reported vulnerabilities
  - people operating major git hosting sites with many users
  - people packaging and distributing git to large numbers of people

where these issues are discussed without risk of the information
leaking out before we're ready to make public announcements.


* Repositories and documentation.

My public git.git repositories are at:

  git://git.kernel.org/pub/scm/git/git.git/
  https://kernel.googlesource.com/pub/scm/git/git
  git://repo.or.cz/alt-git.git/
  https://github.com/git/git/
  git://git.sourceforge.jp/gitroot/git-core/git.git/
  git://git-core.git.sourceforge.net/gitroot/git-core/git-core/

This one shows not just the main integration branches, but also
individual topics broken out:

  git://github.com/gitster/git/

A few web interfaces are found at:

  http://git.kernel.org/cgit/git/git.git
  https://kernel.googlesource.com/pub/scm/git/git
  http://repo.or.cz/w/alt-git.git

Preformatted documentation from the tip of the "master" branch can be
found in:

  git://git.kernel.org/pub/scm/git/git-{htmldocs,manpages}.git/
  git://repo.or.cz/git-{htmldocs,manpages}.git/
  https://github.com/gitster/git-{htmldocs,manpages}.git/

The manual pages formatted in HTML for the tip of 

[ANNOUNCE] Git v2.12.2

[Junio C Hamano]

The latest maintenance release Git v2.12.2 is now available at the
usual places.  These fixes have all been in the 'master' branch to
be included in the next feature release.

The tarballs are found at:

https://www.kernel.org/pub/software/scm/git/

The following public repositories all have a copy of the 'v2.12.2'
tag and the 'maint' branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git



Git v2.12.2 Release Notes
=

Fixes since v2.12.1
---

 * "git status --porcelain" is supposed to give a stable output, but a
   few strings were left as translatable by mistake.

 * "Dumb http" transport used to misparse a nonsense http-alternates
   response, which has been fixed.

 * "git diff --quiet" relies on the size field in diff_filespec to be
   correctly populated, but diff_populate_filespec() helper function
   made an incorrect short-cut when asked only to populate the size
   field for paths that need to go through convert_to_git() (e.g. CRLF
   conversion).

 * There is no need for Python only to give a few messages to the
   standard error stream, but we somehow did.

 * A leak in a codepath to read from a packed object in (rare) cases
   has been plugged.

 * "git upload-pack", which is a counter-part of "git fetch", did not
   report a request for a ref that was not advertised as invalid.
   This is generally not a problem (because "git fetch" will stop
   before making such a request), but is the right thing to do.

 * A "gc.log" file left by a backgrounded "gc --auto" disables further
   automatic gc; it has been taught to run at least once a day (by
   default) by ignoring a stale "gc.log" file that is too old.

 * "git remote rm X", when a branch has remote X configured as the
   value of its branch.*.remote, tried to remove branch.*.remote and
   branch.*.merge and failed if either is unset.

 * A caller of tempfile API that uses stdio interface to write to
   files may ignore errors while writing, which is detected when
   tempfile is closed (with a call to ferror()).  By that time, the
   original errno that may have told us what went wrong is likely to
   be long gone and was overwritten by an irrelevant value.
   close_tempfile() now resets errno to EIO to make errno at least
   predictable.

 * "git show-branch" expected there were only very short branch names
   in the repository and used a fixed-length buffer to hold them
   without checking for overflow.

 * The code that parses header fields in the commit object has been
   updated for (micro)performance and code hygiene.

 * A test that creates a confusing branch whose name is HEAD has been
   corrected not to do so.

 * "Cc:" on the trailer part does not have to conform to RFC strictly,
   unlike in the e-mail header.  "git send-email" has been updated to
   ignore anything after '>' when picking addresses, to allow non-address
   cruft like " # stable 4.4" after the address.

 * "git push" had a handful of codepaths that could lead to a deadlock
   when unexpected error happened, which has been fixed.

 * Code to read submodule..ignore config did not state the
   variable name correctly when giving an error message diagnosing
   misconfiguration.

 * "git ls-remote" and "git archive --remote" are designed to work
   without being in a directory under Git's control.  However, recent
   updates revealed that we randomly look into a directory called
   .git/ without actually doing necessary set-up when working in a
   repository.  Stop doing so.

 * The code to parse the command line "git grep ... 
   [[--] ...]" has been cleaned up, and a handful of bugs
   have been fixed (e.g. we used to check "--" if it is a rev).

 * The code to parse "git -c VAR=VAL cmd" and set configuration
   variable for the duration of cmd had two small bugs, which have
   been fixed.
   This supersedes jc/config-case-cmdline topic that has been discarded.

Also contains various documentation updates and code clean-ups.



Changes since v2.12.1 are as follows:

David Turner (1):
  gc: ignore old gc.log files

Eric Wong (1):
  README: create HTTP/HTTPS links from URLs in Markdown

Jeff King (20):
  grep: move thread initialization a little lower
  grep: re-order rev-parsing loop
  grep: fix "--" rev/pathspec disambiguation
  grep: avoid resolving revision names in --no-index case
  grep: do not diagnose misspelt revs with --no-index
  show-branch: drop head_len variable
  show-branch: store resolved head in heap buffer
  remote: avoid reading $GIT_DIR config in non-repo
  grep: treat revs the same for --untracked as 

Re: How do I copy a branch & its config to a new name?

[Jeff King]

On Fri, Mar 24, 2017 at 12:10:49PM +0100, Ævar Arnfjörð Bjarmason wrote:

> Actually this is a bit confusing, but I think reversing the arguments
> makes sense, i.e.:
> 
> git branch -c dest [src]
> 
> And similarly:
> 
> git checkout -c dest []
> 
> This is confusing in that it reverses the arguments, but more useful
> in that it allows you to omit src like the other invocations of these
> commands, and you can e.g. do:
> 
> git branch -c avar/topic-2
> 
> While on avar/topic to start working on avar/topic-2, which would copy
> avar/topic's state & config.
> 
> I'll put this on my TODO list, but unless someone comes up with a
> compelling argument against the above I plan to make the interface
> look like that.

I think it probably should match "git branch -m", which uses:

  git branch -c [] 

That's the closest operation we currently have, and it still allows
omitting the src. It _is_ different than:

  git branch  []

I wondered if you could advertise "-c" not as a new "copy mode", but
rather as an option for normal branch creation. Sort of a "by the way,
copy the config". But I don't think that's a great mental model.
 doesn't have to be a branch at all.

-Peff

Re: [PATCHv2 00/14] completion: speed up refs completion

[Jeff King]

On Thu, Mar 23, 2017 at 11:28:52AM -0700, Junio C Hamano wrote:

> SZEDER Gábor  writes:
> 
> > This series is the updated version of 'sg/completion-refs-speedup'.
> > It speeds up refs completion for large number of refs, partly by
> > giving up disambiguating ambiguous refs and partly by eliminating most
> > of the shell processing between 'git for-each-ref' and 'ls-remote' and
> > Bash's completion facility.  The rest is a bit of preparatory
> > reorganization, cleanup and bugfixes.
> >
> > Changes since v1:
> > ...
> > [1] - 
> > http://public-inbox.org/git/20170206181545.12869-1-szeder@gmail.com/
> 
> It seems Jacob Keller was the only person who was excited about
> these changes when v1 was posted?  It would be nice to see a bit
> more enthusiasm from other folks who are invested in the completion
> script, but you are the de-facto go-to person on the completion
> already, so ... ;-)
> 
> Will replace.  Let's advance this to 'next' soonish (say, by early
> next week).

I'm far from an expert in the completion scripts, but I read over the
whole thing and it looked good to me. As usual with bash, the
optimizations can make the code a bit non-intuitive, but I think the
changes are clearly explained and the performance results speak for
themselves.

-Peff

Re: [PATCHv2 11/14] completion: let 'for-each-ref' sort remote branches for 'checkout' DWIMery

[Jeff King]

On Thu, Mar 23, 2017 at 04:29:21PM +0100, SZEDER Gábor wrote:

> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 394dcece6..d26312899 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -423,8 +423,9 @@ __git_refs ()
>   # Try to find a remote branch that matches the 
> completion word
>   # but only output if the branch name is unique
>   __git for-each-ref --format="%(refname:strip=3)" \
> + --sort="refname:strip=3" \
>   "refs/remotes/*/$match*" 
> "refs/remotes/*/$match*/**" | \
> - sort | uniq -u
> + uniq -u

I wonder if it would be worth adding a "--unique" option to
for-each-ref. I guess the definition of "unique" may change though (here
you'd want to do it on the sort key, but other cases may want unique
branch names sorted by time, which is less trivial).

-Peff

Re: [PATCHv2 08/14] completion: let 'for-each-ref' and 'ls-remote' filter matching refs

[Jeff King]

On Thu, Mar 23, 2017 at 04:29:18PM +0100, SZEDER Gábor wrote:

> When completing refs, several __git_refs() code paths list all the
> refs from the refs/{heads,tags,remotes}/ hierarchy and then
> __gitcomp_nl() iterates over those refs in a shell loop to filter out
> refs not matching the current ref to be completed.  This comes with a
> considerable performance penalty when a repository contains a lot of
> refs but the current ref can be uniquely completed or when only a
> handful of refs match the current ref.

Part of this is due to outputting too many refs from for-each-ref, but
part of it is that for-each-ref is not good at limiting the refs it
looks at internally. I have a patch for that, but it's not all that
helpful without Michael Haggerty's mmap-packed-refs work, so I'll get it
included there.

So that makes this change doubly important. You get the immediate
speedups, and then you'll get another one when that work is merged
later.

>   case "$cur_" in
>   refs|refs/*)
>   format="refname"
> - refs="${cur_%/*}"
> + refs=("$match*" "$match*/**")
>   track=""

Working on the aforementioned patch, I noticed that for-each-ref's
matching is a little tricky due to its path semantics. So I wanted to
double-check your patterns. :) I think these should do the right thing.

-Peff

Re: [PATCHv2 07/14] completion: don't disambiguate short refs

[Jeff King]

On Thu, Mar 23, 2017 at 04:29:17PM +0100, SZEDER Gábor wrote:

> However, it's questionable whether ambiguous refs are really that bad
> to justify that much extra cost:

It's not clear to me that the existing completion actually does a good
job with disambiguation anyway.

If I have a tag and a branch named "foo", then in theory doing:

  git log fo

should present me with "heads/foo" and "tags/foo" as options. But it
doesn't seem to; it just completes "foo".

But even if it did, those don't _start_ with foo, I have to go to some
work to back up anyway. I think we are better off just completing "foo"
and letting the command complain that it's ambiguous.

So even leaving aside the performance tradeoff, that seems like a more
sensible behavior anyway. And AFAICT, that's the behavior you'd get with
your patch (we'd get two "foo"s, but the completion is presumably smart
enough to handle that.

> This speeds up refs completion considerably.  Uniquely completing a
> branch in a repository with 100k local branches, all packed, best of
> five:
> 
>   On Linux, before:
> 
> $ time __git_complete_refs --cur=maste
> 
> real0m1.662s
> user0m1.368s
> sys 0m0.296s
> 
>   After:
> 
> real0m0.831s
> user0m0.808s
> sys 0m0.028s

This is nice, though I think faster ref storage is another way to attack
the problem. This is much simpler, though. :)

-Peff

Re: [PATCH] push: allow atomic flag via configuration

[Junio C Hamano]

Jeff King  writes:

> My one question would be whether people would want this to actually be
> specific to a particular remote, and not just on for a given repository
> (your "site-specific" in the description made me think of that). In that
> case it would be better as part of the remote.* config.

Yeah, I had the same reaction.  

Conceptually, this sits next to remote.*.push that defines which set
of refs are sent by default, and remote..pushAtomic does make
sense.  If (and only if) it turns out to be cumbersome for somebody
to set the configuration for each and every remote, it is OK to also
add push.atomic to serve as a fallback for remote.*.pushAtomic, I
would think, but adding only push.atomic feels somewhat backwards.

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Junio C Hamano]

Jeff King  writes:

> I see you ended up with a test that uses test_terminal, which is much
> better (and your patch looks good to me).
>
> But I was concerned that there might be a bug in pager_in_use(), so I
> dug into it a little. I think the code there is correct; it's just
> relaying the environment variable's value. Likewise, on the setting
> side, I think the code is correct. We set the environment variable
> whenever we start the pager in setup_pager().
>
> I think what is confusing is that "git -p" does _not_ mean
> "unconditionally use a pager". It means "start a pager if stdout is a
> terminal, even if this command is not usually paged". So something like
> "git -p log >actual" is correct not to trigger pager_in_use().
>
> I also double-checked the documentation, which covers this case
> accurately. So I think all is well, and there's nothing to fix. You
> might want an option for "even though stdout is not a tty pretend like a
> pager is in use", but that is exactly what GIT_PAGER_IN_USE=1 is for.

Thanks ;-).

Re: [PATCH] push: allow atomic flag via configuration

[Jeff King]

On Fri, Mar 24, 2017 at 11:53:54AM -0700, Jonathan Nieder wrote:

> I didn't receive the original patch (maybe mailing delay?) so
> commenting here.

Vger seems a bit slow lately. The list copy did eventually get delivered
to me and public-inbox:

  http://public-inbox.org/git/1490375874.745.227.ca...@locke.gandi.net/

I don't think it answers any of your questions, though. ;)

-Peff

[PATCH] pager_in_use: use git_env_bool()

[Jeff King]

On Fri, Mar 24, 2017 at 02:55:43PM -0400, Jeff King wrote:

> But I was concerned that there might be a bug in pager_in_use(), so I
> dug into it a little. I think the code there is correct; [...]

I did see this small cleanup opportunity, though.

-- >8 --
Subject: [PATCH] pager_in_use: use git_env_bool()

The pager_in_use() function predates git_env_bool(), but
ends up doing the same thing.  Let's make use of the latter,
which is shorter and less repetitive.

Signed-off-by: Jeff King 
---
 pager.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/pager.c b/pager.c
index 73ca8bc3b..c113d898a 100644
--- a/pager.c
+++ b/pager.c
@@ -135,9 +135,7 @@ void setup_pager(void)
 
 int pager_in_use(void)
 {
-   const char *env;
-   env = getenv("GIT_PAGER_IN_USE");
-   return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
+   return git_env_bool("GIT_PAGER_IN_USE", 0);
 }
 
 /*
-- 
2.12.1.843.g1937c56c2

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Jeff King]

On Thu, Mar 23, 2017 at 10:52:34AM -0600, Alex Henrie wrote:

> Unfortunately, I think I found a bug. Even when using `git -p`, the
> function pager_in_use() always returns false if the output is not a
> TTY. So, `isatty(1) || pager_in_use()` and `color_stdout_is_tty ||
> (pager_in_use() && pager_use_color)` are redundant.
> 
> If we want to use `git -p log` in a test, we'll have to change the
> behavior of pager_in_use(). Alternatively, we could use
> `GIT_PAGER_IN_USE=1 git log` instead.

I see you ended up with a test that uses test_terminal, which is much
better (and your patch looks good to me).

But I was concerned that there might be a bug in pager_in_use(), so I
dug into it a little. I think the code there is correct; it's just
relaying the environment variable's value. Likewise, on the setting
side, I think the code is correct. We set the environment variable
whenever we start the pager in setup_pager().

I think what is confusing is that "git -p" does _not_ mean
"unconditionally use a pager". It means "start a pager if stdout is a
terminal, even if this command is not usually paged". So something like
"git -p log >actual" is correct not to trigger pager_in_use().

I also double-checked the documentation, which covers this case
accurately. So I think all is well, and there's nothing to fix. You
might want an option for "even though stdout is not a tty pretend like a
pager is in use", but that is exactly what GIT_PAGER_IN_USE=1 is for.

-Peff

Re: [PATCH] push: allow atomic flag via configuration

[Jonathan Nieder]

Hi,

Jeff King wrote:
> On Fri, Mar 24, 2017 at 06:17:54PM +0100, Romuald Brunet wrote:

>> Added a "push.atomic" option to git-config to allow site-specific
>> configuration of the atomic flag of git push
>
> I don't really use --atomic myself, but this seems like a reasonable
> thing to want, and the implementation looks cleanly done.
>
> My one question would be whether people would want this to actually be
> specific to a particular remote, and not just on for a given repository
> (your "site-specific" in the description made me think of that). In that
> case it would be better as part of the remote.* config.

I didn't receive the original patch (maybe mailing delay?) so
commenting here.

I use --atomic sometimes, but it's tied to the specific context of
what I'm trying to push.

I'd be interested to hear more about the use case.  Do you want pushes
to be atomic opportunistically when the server supports that
(analogous to push.gpgSign=if-asked)?

This also seems likely to break scripts that rely on the current
non-atomic behavior, so without knowing more about the use case it's
hard to see how to proceed.

Thanks and hope that helps,
Jonathan

LETS WORK TOGETHER AND ACHIEVED THIS FUND.

[Mr Joshua Blaise]

Dear Friend,

I need your help transferring (US$4.5M DOLLARS) to your bank account.I have 
every enquiries’details to make the bank believed you and release the fund in 
within 5 banking working days with your full co-operation with me for success.

Send the below requirement to enable me advice you on how to apply to the Bank 
for the claim.

1)Full names.
2)country of origin.
3)Your Mobile No.
4)Your Age.
5)occupation.

Thanks.

Mr Joshua Blaise

Re: [PATCH] push: allow atomic flag via configuration

[Jeff King]

On Fri, Mar 24, 2017 at 06:17:54PM +0100, Romuald Brunet wrote:

> Added a "push.atomic" option to git-config to allow site-specific
> configuration of the atomic flag of git push

I don't really use --atomic myself, but this seems like a reasonable
thing to want, and the implementation looks cleanly done.

My one question would be whether people would want this to actually be
specific to a particular remote, and not just on for a given repository
(your "site-specific" in the description made me think of that). In that
case it would be better as part of the remote.* config.

-Peff

[PATCH v4 13/16] tag: change --point-at to default to HEAD

[Ævar Arnfjörð Bjarmason]

Change the --points-at option to default to HEAD for consistency with
its siblings --contains, --merged etc. which default to
HEAD. Previously we'd get:

$ git tag --points-at 2>&1 | head -n 1
error: option `points-at' requires a value

This changes behavior added in commit ae7706b9ac (tag: add --points-at
list option, 2012-02-08).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-tag.txt | 3 ++-
 builtin/tag.c | 3 ++-
 t/t7004-tag.sh| 9 -
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 4d289f5dd5..d5cdb18d96 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -139,7 +139,8 @@ This option is only applicable when listing tags without 
annotation lines.
commit (`HEAD` if not specified), incompatible with `--merged`.
 
 --points-at ::
-   Only list tags of the given object. Implies `--list`.
+   Only list tags of the given object (HEAD if not
+   specified). Implies `--list`.
 
 -m ::
 --message=::
diff --git a/builtin/tag.c b/builtin/tag.c
index 3c686961db..8bf6d85176 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -431,7 +431,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 N_("field name to sort on"), 
_opt_ref_sorting),
{
OPTION_CALLBACK, 0, "points-at", _at, 
N_("object"),
-   N_("print only tags of the object"), 0, 
parse_opt_object_name
+   N_("print only tags of the object"), 
PARSE_OPT_LASTARG_DEFAULT,
+   parse_opt_object_name, (intptr_t) "HEAD"
},
OPT_STRING(  0 , "format", , N_("format"), N_("format to 
use for the output")),
OPT_BOOL('i', "ignore-case", , N_("sorting and filtering 
are case insensitive")),
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 5823de16aa..3529c3009c 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1534,7 +1534,8 @@ do
"
test_expect_success "Doing 'git tag --list-like $option  
 is permitted" "
git tag -n $option HEAD HEAD &&
-   git tag $option HEAD HEAD
+   git tag $option HEAD HEAD &&
+   git tag $option
"
 done
 
@@ -1546,6 +1547,12 @@ test_expect_success '--points-at can be used in non-list 
mode' '
test_cmp expect actual
 '
 
+test_expect_success '--points-at is a synonym for --points-at HEAD' '
+   echo v4.0 >expect &&
+   git tag --points-at >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success '--points-at finds lightweight tags' '
echo v4.0 >expect &&
git tag --points-at v4.0 >actual &&
-- 
2.11.0

[PATCH v4 15/16] ref-filter: reflow recently changed branch/tag/for-each-ref docs

[Ævar Arnfjörð Bjarmason]

Reflow the recently changed branch/tag-for-each-ref
documentation. This change shows no changes under --word-diff, except
the innocuous change of moving git-tag.txt's "[--sort=]" around
slightly.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-branch.txt | 15 ---
 Documentation/git-tag.txt|  7 ---
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e4b5d5c3e1..5e175ec339 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -10,9 +10,9 @@ SYNOPSIS
 [verse]
 'git branch' [--color[=] | --no-color] [-r | -a]
[--list] [-v [--abbrev= | --no-abbrev]]
-   [--column[=] | --no-column]
+   [--column[=] | --no-column] [--sort=]
[(--merged | --no-merged) []]
-   [--contains [

[PATCH v4 16/16] tag: add tests for --with and --without

[Ævar Arnfjörð Bjarmason]

Change the test suite to test for these synonyms for --contains and
--no-contains, respectively.

Before this change there were no tests for them at all. This doesn't
exhaustively test for them as well as their --contains and
--no-contains synonyms, but at least it's something.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t7004-tag.sh | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 8a6e8032da..6143113dbb 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1599,10 +1599,12 @@ test_expect_success 'mixing incompatibles modes and 
options is forbidden' '
test_must_fail git tag --contains tag-blob &&
test_must_fail git tag --no-contains tag-tree &&
test_must_fail git tag --no-contains tag-blob &&
-   test_must_fail git tag --contains --no-contains
+   test_must_fail git tag --contains --no-contains &&
+   test_must_fail git tag --no-with HEAD &&
+   test_must_fail git tag --no-without HEAD
 '
 
-for option in --contains --no-contains --merged --no-merged --points-at
+for option in --contains --with --no-contains --without --merged --no-merged 
--points-at
 do
test_expect_success "mixing incompatible modes with $option is 
forbidden" "
test_must_fail git tag -d $option HEAD &&
-- 
2.11.0

[PATCH v4 07/16] tag tests: fix a typo in a test description

[Ævar Arnfjörð Bjarmason]

Change "suceed" to "succeed" in a test description. The typo has been
here since the code was originally added in commit ef5a6fb597 ("Add
test-script for git-tag", 2007-06-28).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t7004-tag.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 830eff948e..63ee2cf727 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -136,7 +136,7 @@ test_expect_success \
'test $(git tag -l mytag) = mytag'
 
 test_expect_success \
-   'listing tags using a non-matching pattern should suceed' \
+   'listing tags using a non-matching pattern should succeed' \
'git tag -l xxx'
 
 test_expect_success \
-- 
2.11.0

[PATCH v4 14/16] ref-filter: add --no-contains option to tag/branch/for-each-ref

[Ævar Arnfjörð Bjarmason]

Change the tag, branch & for-each-ref commands to have a --no-contains
option in addition to their longstanding --contains options.

This allows for finding the last-good rollout tag given a known-bad
. Given a hypothetically bad commit cf5c7253e0, the git
version to revert to can be found with this hacky two-liner:

(git tag -l 'v[0-9]*'; git tag -l --contains cf5c7253e0 'v[0-9]*') |
sort | uniq -c | grep -E '^ *1 ' | awk '{print $2}' | tail -n 10

With this new --no-contains option the same can be achieved with:

git tag -l --no-contains cf5c7253e0 'v[0-9]*' | sort | tail -n 10

As the filtering machinery is shared between the tag, branch &
for-each-ref commands, implement this for those commands too. A
practical use for this with "branch" is e.g. finding branches which
were branched off between v2.8.0 and v2.10.0:

git branch --contains v2.8.0 --no-contains v2.10.0

The "describe" command also has a --contains option, but its semantics
are unrelated to what tag/branch/for-each-ref use --contains for. A
--no-contains option for "describe" wouldn't make any sense, other
than being exactly equivalent to not supplying --contains at all,
which would be confusing at best.

Add a --without option to "tag" as an alias for --no-contains, for
consistency with --with and --contains.  The --with option is
undocumented, and possibly the only user of it is
Junio (). But it's
trivial to support, so let's do that.

The additions to the the test suite are inverse copies of the
corresponding --contains tests. With this change --no-contains for
tag, branch & for-each-ref is just as well tested as the existing
--contains option.

In addition to those tests, add a test for "tag" which asserts that
--no-contains won't find tree/blob tags, which is slightly
unintuitive, but consistent with how --contains works & is documented.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-branch.txt   |  16 +++-
 Documentation/git-for-each-ref.txt |   6 +-
 Documentation/git-tag.txt  |   6 +-
 builtin/branch.c   |   5 +-
 builtin/for-each-ref.c |   3 +-
 builtin/tag.c  |   8 +-
 contrib/completion/git-completion.bash |   4 +-
 parse-options.h|   2 +
 ref-filter.c   |  19 +++--
 ref-filter.h   |   1 +
 t/t3201-branch-contains.sh |  54 +-
 t/t6302-for-each-ref-filter.sh |  16 
 t/t7004-tag.sh | 130 -
 13 files changed, 245 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index e465298571..e4b5d5c3e1 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -11,7 +11,8 @@ SYNOPSIS
 'git branch' [--color[=] | --no-color] [-r | -a]
[--list] [-v [--abbrev= | --no-abbrev]]
[--column[=] | --no-column]
-   [(--merged | --no-merged | --contains) []] [--sort=]
+   [(--merged | --no-merged) []]
+   [--contains [

[PATCH v4 12/16] tag: implicitly supply --list given another list-like option

[Ævar Arnfjörð Bjarmason]

Change the "tag" command to implicitly turn on its --list mode when
provided with a list-like option such as --contains, --points-at etc.

This is for consistency with how "branch" works. When "branch" is
given a list-like option, such as --contains, it implicitly provides
--list. Before this change "tag" would error out on those sorts of
invocations. I.e. while both of these worked for "branch":

git branch --contains v2.8.0 
git branch --list --contains v2.8.0 

Only the latter form worked for "tag":

git tag --contains v2.8.0 '*rc*'
git tag --list --contains v2.8.0 '*rc*'

Now "tag", like "branch", will implicitly supply --list when a
list-like option is provided, and no other conflicting non-list
options (such as -d) are present on the command-line.

Spelunking through the history via:

git log --reverse -p -G'only allowed with' -- '*builtin*tag*c'

Reveals that there was no good reason for not allowing this in the
first place. The --contains option added in 32c35cfb1e ("git-tag: Add
--contains option", 2009-01-26) made this an error. All the other
subsequent list-like options that were added copied its pattern of
making this usage an error.

The only tests that break as a result of this change are tests that
were explicitly checking that this "branch-like" usage wasn't
permitted. Change those failing tests to check that this invocation
mode is permitted, add extra tests for the list-like options we
weren't testing, and tests to ensure that e.g. we don't toggle the
list mode in the presence of other conflicting non-list options.

With this change errors messages such as "--contains option is only
allowed with -l" don't make sense anymore, since options like
--contain turn on -l. Instead we error out when list-like options such
as --contain are used in conjunction with conflicting options such as
-d or -v.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-tag.txt | 17 +--
 builtin/tag.c | 18 ++--
 t/t7004-tag.sh| 55 ++-
 3 files changed, 73 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 47d8733523..4d289f5dd5 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -82,10 +82,11 @@ OPTIONS
 
 -n::
 specifies how many lines from the annotation, if any,
-   are printed when using -l.
-   The default is not to print any annotation lines.
-   If no number is given to `-n`, only the first line is printed.
-   If the tag is not annotated, the commit message is displayed instead.
+   are printed when using -l. Implies `--list`.
++
+The default is not to print any annotation lines.
+If no number is given to `-n`, only the first line is printed.
+If the tag is not annotated, the commit message is displayed instead.
 
 -l::
 --list::
@@ -95,6 +96,10 @@ OPTIONS
 Running "git tag" without arguments also lists all tags. The pattern
 is a shell wildcard (i.e., matched using fnmatch(3)). Multiple
 patterns may be given; if any of them matches, the tag is shown.
++
+This option is implicitly supplied if any other list-like option such
+as `--contains` is provided. See the documentation for each of those
+options for details.
 
 --sort=::
Sort based on the key given.  Prefix `-` to sort in
@@ -123,7 +128,7 @@ This option is only applicable when listing tags without 
annotation lines.
 
 --contains []::
Only list tags which contain the specified commit (HEAD if not
-   specified).
+   specified). Implies `--list`.
 
 --merged []::
Only list tags whose commits are reachable from the specified
@@ -134,7 +139,7 @@ This option is only applicable when listing tags without 
annotation lines.
commit (`HEAD` if not specified), incompatible with `--merged`.
 
 --points-at ::
-   Only list tags of the given object.
+   Only list tags of the given object. Implies `--list`.
 
 -m ::
 --message=::
diff --git a/builtin/tag.c b/builtin/tag.c
index ad29be6923..3c686961db 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -454,8 +454,14 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
}
create_tag_object = (opt.sign || annotate || msg.given || msgfile);
 
-   if (argc == 0 && !cmdmode)
-   cmdmode = 'l';
+   if (!cmdmode) {
+   if (argc == 0)
+   cmdmode = 'l';
+   else if (filter.with_commit ||
+filter.points_at.nr || filter.merge_commit ||
+filter.lines != -1)
+   cmdmode = 'l';
+   }
 
if ((create_tag_object || force) && (cmdmode != 0))
usage_with_options(git_tag_usage, options);
@@ -485,13 +491,13 @@ int cmd_tag(int argc, const char **argv, const char 
*prefix)
return ret;
}
if (filter.lines != -1)
-   die(_("-n 

[PATCH v4 11/16] tag: change misleading --list documentation

[Ævar Arnfjörð Bjarmason]

Change the documentation for --list so that it's described as a
toggle, not as an option that takes a  as an argument.

Junio initially documented this in b867c7c23a ("git-tag: -l to list
tags (usability).", 2006-02-17), but later Jeff King changed "tag" to
accept multiple patterns in 588d0e834b ("tag: accept multiple patterns
for --list", 2011-06-20).

However, documenting this as "-l " was never correct, as
these both worked before Jeff's change:

git tag -l 'v*'
git tag 'v*' -l

One would expect an option that was documented like that to only
accept:

git tag --list
git tag --list 'v*rc*'

And after Jeff's change, one that took multiple patterns:

git tag --list 'v*rc*' --list '*2.8*'

But since it's actually a toggle all of these work as well, and
produce identical output to the last example above:

git tag --list 'v*rc*' '*2.8*'
git tag --list 'v*rc*' '*2.8*' --list --list --list
git tag --list 'v*rc*' '*2.8*' --list -l --list -l --list

Now the documentation is more in tune with how the "branch" command
describes its --list option since commit cddd127b9a ("branch:
introduce --list option", 2011-08-28).

Change the test suite to assert that these invocations work for the
cases that weren't already being tested for.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-tag.txt | 15 ---
 t/t7004-tag.sh| 25 +
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 448fdf3743..47d8733523 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -87,13 +87,14 @@ OPTIONS
If no number is given to `-n`, only the first line is printed.
If the tag is not annotated, the commit message is displayed instead.
 
--l ::
---list ::
-   List tags with names that match the given pattern (or all if no
-   pattern is given).  Running "git tag" without arguments also
-   lists all tags. The pattern is a shell wildcard (i.e., matched
-   using fnmatch(3)).  Multiple patterns may be given; if any of
-   them matches, the tag is shown.
+-l::
+--list::
+   List tags. With optional `...`, e.g. `git tag --list
+   'v-*'`, list only the tags that match the pattern(s).
++
+Running "git tag" without arguments also lists all tags. The pattern
+is a shell wildcard (i.e., matched using fnmatch(3)). Multiple
+patterns may be given; if any of them matches, the tag is shown.
 
 --sort=::
Sort based on the key given.  Prefix `-` to sort in
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 92af8bb7e6..75681b2cad 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -118,6 +118,18 @@ test_expect_success 'listing all tags if one exists should 
succeed' '
git tag
 '
 
+cat >expect actual &&
+   test_cmp expect actual &&
+   git tag --list -l --list >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'listing all tags if one exists should output that tag' '
test $(git tag -l) = mytag &&
test $(git tag) = mytag
@@ -336,6 +348,19 @@ test_expect_success 'tag -l can accept multiple patterns' '
test_cmp expect actual
 '
 
+# Between v1.7.7 & v2.13.0 a fair reading of the git-tag documentation
+# could leave you with the impression that "-l  -l "
+# was how we wanted to accept multiple patterns.
+#
+# This test should not imply that this is a sane thing to support. but
+# since the documentation was worded like it was let's at least find
+# out if we're going to break this long-documented form of taking
+# multiple patterns.
+test_expect_success 'tag -l  -l  works, as our buggy 
documentation previously suggested' '
+   git tag -l "v1*" -l "v0*" >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'listing tags in column' '
COLUMNS=40 git tag -l --column=row >actual &&
cat >expected <<\EOF &&
-- 
2.11.0

[PATCH v4 05/16] ref-filter: add test for --contains on a non-commit

[Ævar Arnfjörð Bjarmason]

Change the tag test suite to test for --contains on a tree & blob. It
only accepts commits and will spew out " is a tree, not a
commit".

It's sufficient to test this just for the "tag" and "branch" commands,
because it covers all the machinery shared between "branch" and
"for-each-ref".

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t3201-branch-contains.sh | 9 +
 t/t7004-tag.sh | 4 +++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
index 7f3ec47241..daa3ae82b7 100755
--- a/t/t3201-branch-contains.sh
+++ b/t/t3201-branch-contains.sh
@@ -130,6 +130,15 @@ test_expect_success 'implicit --list conflicts with 
modification options' '
 
 '
 
+test_expect_success 'Assert that --contains only works on commits, not trees & 
blobs' '
+   test_must_fail git branch --contains master^{tree} &&
+   blob=$(git hash-object -w --stdin <<-\EOF
+   Some blob
+   EOF
+   ) &&
+   test_must_fail git branch --contains $blob
+'
+
 # We want to set up a case where the walk for the tracking info
 # of one branch crosses the tip of another branch (and make sure
 # that the latter walk does not mess up our flag to see if it was
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 45790664c1..3439913488 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1461,7 +1461,9 @@ test_expect_success 'mixing incompatibles modes and 
options is forbidden' '
test_must_fail git tag -n 100 &&
test_must_fail git tag -l -m msg &&
test_must_fail git tag -l -F some file &&
-   test_must_fail git tag -v -s
+   test_must_fail git tag -v -s &&
+   test_must_fail git tag --contains tag-tree &&
+   test_must_fail git tag --contains tag-blob
 '
 
 # check points-at
-- 
2.11.0

[PATCH v4 04/16] ref-filter: make combining --merged & --no-merged an error

[Ævar Arnfjörð Bjarmason]

Change the behavior of specifying --merged & --no-merged to be an
error, instead of silently picking the option that was provided last.

Subsequent changes of mine add a --no-contains option in addition to
the existing --contains. Providing both of those isn't an error, and
has actual meaning.

Making its cousins have different behavior in this regard would be
confusing to the user, especially since we'd be silently disregarding
some of their command-line input.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-branch.txt   |  6 --
 Documentation/git-for-each-ref.txt |  6 --
 Documentation/git-tag.txt  |  4 ++--
 ref-filter.c   | 11 ++-
 t/t3200-branch.sh  |  4 
 t/t6302-for-each-ref-filter.sh |  4 
 t/t7004-tag.sh |  4 
 7 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 092f1bcf9f..e465298571 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -215,11 +215,13 @@ start-point is either a local or remote-tracking branch.
 
 --merged []::
Only list branches whose tips are reachable from the
-   specified commit (HEAD if not specified). Implies `--list`.
+   specified commit (HEAD if not specified). Implies `--list`,
+   incompatible with `--no-merged`.
 
 --no-merged []::
Only list branches whose tips are not reachable from the
-   specified commit (HEAD if not specified). Implies `--list`.
+   specified commit (HEAD if not specified). Implies `--list`,
+   incompatible with `--merged`.
 
 ::
The name of the branch to create or delete.
diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 111e1be6f5..4d55893712 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -69,11 +69,13 @@ OPTIONS
 
 --merged []::
Only list refs whose tips are reachable from the
-   specified commit (HEAD if not specified).
+   specified commit (HEAD if not specified),
+   incompatible with `--no-merged`.
 
 --no-merged []::
Only list refs whose tips are not reachable from the
-   specified commit (HEAD if not specified).
+   specified commit (HEAD if not specified),
+   incompatible with `--merged`.
 
 --contains []::
Only list refs which contain the specified commit (HEAD if not
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 3abf912782..448fdf3743 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -126,11 +126,11 @@ This option is only applicable when listing tags without 
annotation lines.
 
 --merged []::
Only list tags whose commits are reachable from the specified
-   commit (`HEAD` if not specified).
+   commit (`HEAD` if not specified), incompatible with `--no-merged`.
 
 --no-merged []::
Only list tags whose commits are not reachable from the specified
-   commit (`HEAD` if not specified).
+   commit (`HEAD` if not specified), incompatible with `--merged`.
 
 --points-at ::
Only list tags of the given object.
diff --git a/ref-filter.c b/ref-filter.c
index 9c82b5b9d6..d7efae7b53 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2084,8 +2084,17 @@ int parse_opt_merge_filter(const struct option *opt, 
const char *arg, int unset)
 {
struct ref_filter *rf = opt->value;
unsigned char sha1[20];
+   int no_merged = starts_with(opt->long_name, "no");
 
-   rf->merge = starts_with(opt->long_name, "no")
+   if (rf->merge) {
+   if (no_merged) {
+   return opterror(opt, "is incompatible with --merged", 
0);
+   } else {
+   return opterror(opt, "is incompatible with 
--no-merged", 0);
+   }
+   }
+
+   rf->merge = no_merged
? REF_FILTER_MERGED_OMIT
: REF_FILTER_MERGED_INCLUDE;
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 9f353c0efc..fe62e7c775 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -978,6 +978,10 @@ test_expect_success '--merged catches invalid object 
names' '
test_must_fail git branch --merged 

 '
 
+test_expect_success '--merged is incompatible with --no-merged' '
+   test_must_fail git branch --merged HEAD --no-merged HEAD
+'
+
 test_expect_success 'tracking with unexpected .fetch refspec' '
rm -rf a b c d &&
git init a &&
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index a09a1a46ef..d36d5dc124 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -421,4 +421,8 @@ test_expect_success 'check %(if:notequals=)' '
test_cmp expect actual
 '
 
+test_expect_success '--merged is incompatible with --no-merged' '
+   test_must_fail git 

[PATCH v4 02/16] tag doc: split up the --[no-]merged documentation

[Ævar Arnfjörð Bjarmason]

Split up the --[no-]merged documentation into documentation that
documents each option independently. This is in line with how "branch"
and "for-each-ref" are documented, and makes subsequent changes to
discuss the limits & caveats of each option easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-tag.txt | 11 +++
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 33f18ea5fb..68b0ab2410 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -124,10 +124,13 @@ This option is only applicable when listing tags without 
annotation lines.
Only list tags which contain the specified commit (HEAD if not
specified).
 
---[no-]merged []::
-   Only list tags whose tips are reachable, or not reachable
-   if `--no-merged` is used, from the specified commit (`HEAD`
-   if not specified).
+--merged []::
+   Only list tags whose tips are reachable from the specified commit
+   (`HEAD` if not specified).
+
+--no-merged []::
+   Only list tags whose tips are not reachable from the specified
+   commit (`HEAD` if not specified).
 
 --points-at ::
Only list tags of the given object.
-- 
2.11.0

[PATCH v4 03/16] tag doc: reword --[no-]merged to talk about commits, not tips

[Ævar Arnfjörð Bjarmason]

Change the wording for the --merged and --no-merged options to talk
about "commits" instead of "tips".

This phrasing was copied from the "branch" documentation in commit
5242860f54 ("tag.c: implement '--merged' and '--no-merged' options",
2015-09-10). Talking about the "tip" is branch nomenclature, not
something usually associated with tags.

This phrasing might lead the reader to believe that these options
might find tags pointing to trees or blobs, let's instead be explicit
and only talk about commits.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-tag.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 68b0ab2410..3abf912782 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -125,11 +125,11 @@ This option is only applicable when listing tags without 
annotation lines.
specified).
 
 --merged []::
-   Only list tags whose tips are reachable from the specified commit
-   (`HEAD` if not specified).
+   Only list tags whose commits are reachable from the specified
+   commit (`HEAD` if not specified).
 
 --no-merged []::
-   Only list tags whose tips are not reachable from the specified
+   Only list tags whose commits are not reachable from the specified
commit (`HEAD` if not specified).
 
 --points-at ::
-- 
2.11.0

[PATCH v4 01/16] tag doc: move the description of --[no-]merged earlier

[Ævar Arnfjörð Bjarmason]

Move the documentation for the --merged & --no-merged options earlier
in the documentation, to sit along the other switches, and right next
to the similar --contains and --points-at switches.

It makes more sense to group the options together, not have some
options after the like of , ,  etc.

This was originally put there when the --merged & --no-merged options
were introduced in 5242860f54 ("tag.c: implement '--merged' and
'--no-merged' options", 2015-09-10). It's not apparent from that
commit that the documentation is being placed apart from other
options, rather than along with them, so this was likely missed in the
initial review.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Documentation/git-tag.txt | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 525737a5d8..33f18ea5fb 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -124,6 +124,11 @@ This option is only applicable when listing tags without 
annotation lines.
Only list tags which contain the specified commit (HEAD if not
specified).
 
+--[no-]merged []::
+   Only list tags whose tips are reachable, or not reachable
+   if `--no-merged` is used, from the specified commit (`HEAD`
+   if not specified).
+
 --points-at ::
Only list tags of the given object.
 
@@ -173,11 +178,6 @@ This option is only applicable when listing tags without 
annotation lines.
that of linkgit:git-for-each-ref[1].  When unspecified,
defaults to `%(refname:strip=2)`.
 
---[no-]merged []::
-   Only list tags whose tips are reachable, or not reachable
-   if `--no-merged` is used, from the specified commit (`HEAD`
-   if not specified).
-
 CONFIGURATION
 -
 By default, 'git tag' in sign-with-default mode (-s) will use your
-- 
2.11.0

[PATCH v4 09/16] tag: add more incompatibles mode tests

[Ævar Arnfjörð Bjarmason]

Amend the test suite to test for more invalid uses like "-l -a"
etc.

This change tests the code path in builtin/tag.c between lines:

if (argc == 0 && !cmdmode)

And:

if ((create_tag_object || force) && (cmdmode != 0))

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t7004-tag.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 63ee2cf727..92af8bb7e6 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1455,8 +1455,24 @@ test_expect_success 'checking that initial commit is in 
all tags' "
 
 test_expect_success 'mixing incompatibles modes and options is forbidden' '
test_must_fail git tag -a &&
+   test_must_fail git tag -a -l &&
+   test_must_fail git tag -s &&
+   test_must_fail git tag -s -l &&
+   test_must_fail git tag -m &&
+   test_must_fail git tag -m -l &&
+   test_must_fail git tag -m "hlagh" &&
+   test_must_fail git tag -m "hlagh" -l &&
+   test_must_fail git tag -F &&
+   test_must_fail git tag -F -l &&
+   test_must_fail git tag -f &&
+   test_must_fail git tag -f -l &&
+   test_must_fail git tag -a -s -m -F &&
+   test_must_fail git tag -a -s -m -F -l &&
test_must_fail git tag -l -v &&
+   test_must_fail git tag -l -d &&
+   test_must_fail git tag -l -v -d &&
test_must_fail git tag -n 100 &&
+   test_must_fail git tag -n 100 -v &&
test_must_fail git tag -l -m msg &&
test_must_fail git tag -l -F some file &&
test_must_fail git tag -v -s &&
-- 
2.11.0

[PATCH v4 08/16] for-each-ref: partly change to in help

[Ævar Arnfjörð Bjarmason]

Change mentions of  to  in the help output of
for-each-ref as appropriate.

Both --[no-]merged and --contains only take commits, but --points-at
can take any object, such as a tag pointing to a tree or blob.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/for-each-ref.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index df41fa0350..1a5ed20f59 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -8,8 +8,8 @@
 static char const * const for_each_ref_usage[] = {
N_("git for-each-ref [] []"),
N_("git for-each-ref [--points-at ]"),
-   N_("git for-each-ref [(--merged | --no-merged) []]"),
-   N_("git for-each-ref [--contains []]"),
+   N_("git for-each-ref [(--merged | --no-merged) []]"),
+   N_("git for-each-ref [--contains []]"),
NULL
 };
 
-- 
2.11.0

[PATCH v4 06/16] tag: remove a TODO item from the test suite

[Ævar Arnfjörð Bjarmason]

Change the test for "git tag -l" to not have an associated TODO
comment saying that it should return non-zero if there's no tags.

This was added in commit ef5a6fb597 ("Add test-script for git-tag",
2007-06-28) when the tests for "tag" were initially added, but at this
point changing this would be inconsistent with how "git tag" is a
synonym for "git tag -l", and would needlessly break external code
that relies on this porcelain command.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 t/t7004-tag.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 3439913488..830eff948e 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -16,7 +16,6 @@ tag_exists () {
git show-ref --quiet --verify refs/tags/"$1"
 }
 
-# todo: git tag -l now returns always zero, when fixed, change this test
 test_expect_success 'listing all tags in an empty tree should succeed' '
git tag -l &&
git tag
@@ -136,7 +135,6 @@ test_expect_success \
'listing a tag using a matching pattern should output that tag' \
'test $(git tag -l mytag) = mytag'
 
-# todo: git tag -l now returns always zero, when fixed, change this test
 test_expect_success \
'listing tags using a non-matching pattern should suceed' \
'git tag -l xxx'
-- 
2.11.0

[PATCH v4 10/16] parse-options: add OPT_NONEG to the "contains" option

[Ævar Arnfjörð Bjarmason]

Add the OPT_NONEG flag to the "contains" option and its hidden synonym
"with". Since this was added in commit 694a577519 ("git-branch
--contains=commit", 2007-11-07) giving --no-{contains,with} hasn't
been an error, but has emitted the help output since
filter.with_commit wouldn't get set.

Now git will emit "error: unknown option `no-{contains,with}'" at the
top of the help output.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 parse-options.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/parse-options.h b/parse-options.h
index dcd8a0926c..9f48f554ba 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -258,7 +258,7 @@ extern int parse_opt_passthru_argv(const struct option *, 
const char *, int);
  PARSE_OPT_LASTARG_DEFAULT | flag, \
  parse_opt_commits, (intptr_t) "HEAD" \
}
-#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, 0)
-#define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH("with", v, h, PARSE_OPT_HIDDEN)
+#define OPT_CONTAINS(v, h) _OPT_CONTAINS_OR_WITH("contains", v, h, 
PARSE_OPT_NONEG)
+#define OPT_WITH(v, h) _OPT_CONTAINS_OR_WITH("with", v, h, PARSE_OPT_HIDDEN | 
PARSE_OPT_NONEG)
 
 #endif
-- 
2.11.0

[PATCH v4 00/16] Various changes to the "tag" command & related

[Ævar Arnfjörð Bjarmason]

Hopefully the final version. This is exactly like v3 except for a
couple of minor changes (and rebased on the latest upstream master):


Ævar Arnfjörð Bjarmason (16):
  tag doc: move the description of --[no-]merged earlier
  tag doc: split up the --[no-]merged documentation
  tag doc: reword --[no-]merged to talk about commits, not tips
  ref-filter: make combining --merged & --no-merged an error
  ref-filter: add test for --contains on a non-commit
  tag: remove a TODO item from the test suite
  tag tests: fix a typo in a test description
  for-each-ref: partly change  to  in help
  tag: add more incompatibles mode tests

Clarified in the commit message what code in builtin/tag.c we're
trying to test here.

  parse-options: add OPT_NONEG to the "contains" option
  tag: change misleading --list  documentation

Use the same phrasing as Junio's "branch doc: update description for
`--list`" does when describing "git tag --list".

  tag: implicitly supply --list given another list-like option
  tag: change --point-at to default to HEAD
  ref-filter: add --no-contains option to tag/branch/for-each-ref
  ref-filter: reflow recently changed branch/tag/for-each-ref docs
  tag: add tests for --with and --without

 Documentation/git-branch.txt   |  33 +++--
 Documentation/git-for-each-ref.txt |  12 +-
 Documentation/git-tag.txt  |  59 +---
 builtin/branch.c   |   5 +-
 builtin/for-each-ref.c |   5 +-
 builtin/tag.c  |  27 ++--
 contrib/completion/git-completion.bash |   4 +-
 parse-options.h|   6 +-
 ref-filter.c   |  30 +++-
 ref-filter.h   |   1 +
 t/t3200-branch.sh  |   4 +
 t/t3201-branch-contains.sh |  61 +++-
 t/t6302-for-each-ref-filter.sh |  20 +++
 t/t7004-tag.sh | 245 +++--
 14 files changed, 440 insertions(+), 72 deletions(-)

-- 
2.11.0

Re: [PATCH v2] log: if --decorate is not given, default to --decorate=auto

[Jonathan Nieder]

Alex Henrie wrote:

> Signed-off-by: Alex Henrie 
> ---
>  builtin/log.c  |  9 -
>  t/t4202-log.sh | 10 +-
>  2 files changed, 17 insertions(+), 2 deletions(-)

Nice.

Reviewed-by: Jonathan Nieder 

[PATCH 5/7] submodule.c: stricter checking for submodules in is_submodule_modified

[Stefan Beller]

By having a stricter check in the superproject we catch errors earlier,
instead of spawning a child process to tell us.

Reviewed-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 submodule.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/submodule.c b/submodule.c
index 256f15fde1..467f1de763 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1052,11 +1052,12 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
git_dir = read_gitfile(buf.buf);
if (!git_dir)
git_dir = buf.buf;
-   if (!is_directory(git_dir)) {
+   if (!is_git_directory(git_dir)) {
+   if (is_directory(git_dir))
+   die(_("'%s' not recognized as a git repository"), 
git_dir);
strbuf_release();
/* The submodule is not checked out, so it is not modified */
return 0;
-
}
strbuf_reset();
 
-- 
2.12.1.437.g2b7623d507

[PATCH 7/7] submodule.c: correctly handle nested submodules in is_submodule_modified

[Stefan Beller]

When a nested submodule has untracked files, it would be reported as
"modified submodule" in the superproject, because submodules are not
parsed correctly in is_submodule_modified as they are bucketed into
the modified pile as "they are not an untracked file".

Signed-off-by: Stefan Beller 
---
 submodule.c | 16 ++--
 t/t3600-rm.sh   |  2 +-
 t/t7506-status-submodule.sh |  2 +-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index 467f1de763..ec7e9b1b06 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1078,8 +1078,20 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
/* regular untracked files */
if (buf.buf[0] == '?')
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-   else
-   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+
+   /* regular unmerged and renamed files */
+   if (buf.buf[0] == 'u' ||
+   buf.buf[0] == '1' ||
+   buf.buf[0] == '2') {
+   if (buf.buf[5] == 'S') {
+   /* nested submodule handling */
+   if (buf.buf[6] == 'C' || buf.buf[7] == 'M')
+   dirty_submodule |= 
DIRTY_SUBMODULE_MODIFIED;
+   if (buf.buf[8] == 'U')
+   dirty_submodule |= 
DIRTY_SUBMODULE_UNTRACKED;
+   } else
+   dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
+   }
 
if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || 
ignore_untracked)) {
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index a6e5c5bd56..b58793448b 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -659,7 +659,7 @@ test_expect_success 'rm of a populated nested submodule 
with nested untracked fi
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified_inside actual &&
+   test_cmp expect.modified_untracked actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index ad46384064..e3cdcede72 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -324,7 +324,7 @@ test_expect_success 'status with untracked file in nested 
submodule (porcelain)'
 test_expect_success 'status with untracked file in nested submodule (short)' '
git -C super status --short >output &&
diff output - <<-\EOF
-m sub1
+? sub1
EOF
 '
 
-- 
2.12.1.437.g2b7623d507

[PATCH 3/7] submodule.c: convert is_submodule_modified to use strbuf_getwholeline

[Stefan Beller]

Instead of implementing line reading yet again, make use of our beautiful
library function to read one line.  By using strbuf_getwholeline instead
of strbuf_read, we avoid having to allocate memory for the entire child
process output at once.  That is, we limit maximum memory usage.
Once we know all information that we care about, we can terminate
the child early. In that case we do not care about its exit code as well.

Helped-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 submodule.c | 31 +++
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/submodule.c b/submodule.c
index e52cb8a958..0c43f9f2b1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1041,12 +1041,12 @@ int fetch_populated_submodules(const struct argv_array 
*options,
 
 unsigned is_submodule_modified(const char *path, int ignore_untracked)
 {
-   ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
struct strbuf buf = STRBUF_INIT;
+   FILE *fp;
unsigned dirty_submodule = 0;
-   const char *line, *next_line;
const char *git_dir;
+   int ignore_cp_exit_code = 0;
 
strbuf_addf(, "%s/.git", path);
git_dir = read_gitfile(buf.buf);
@@ -1072,28 +1072,27 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
if (start_command())
die("Could not run 'git status --porcelain' in submodule %s", 
path);
 
-   len = strbuf_read(, cp.out, 1024);
-   line = buf.buf;
-   while (len > 2) {
-   if ((line[0] == '?') && (line[1] == '?'))
+   fp = xfdopen(cp.out, "r");
+   while (strbuf_getwholeline(, fp, '\n') != EOF) {
+   if ((buf.buf[0] == '?') && (buf.buf[1] == '?'))
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
else
dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
 
if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
-   ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || 
ignore_untracked))
-   break;
-
-   next_line = strchr(line, '\n');
-   if (!next_line)
+   ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || 
ignore_untracked)) {
+   /*
+* We're not interested in any further information from
+* the child any more, no output nor its exit code.
+*/
+   kill(cp.pid, SIGTERM);
+   ignore_cp_exit_code = 1;
break;
-   next_line++;
-   len -= (next_line - line);
-   line = next_line;
+   }
}
-   close(cp.out);
+   fclose(fp);
 
-   if (finish_command())
+   if (finish_command() && !ignore_cp_exit_code)
die("'git status --porcelain' failed in submodule %s", path);
 
strbuf_release();
-- 
2.12.1.437.g2b7623d507

[PATCH 4/7] submodule.c: port is_submodule_modified to use porcelain 2

[Stefan Beller]

Migrate 'is_submodule_modified' to the new porcelain format of
git-status. This conversion attempts to convert faithfully, i.e.
the behavior ought to be exactly the same.

As the output in the parsing only distinguishes between untracked files
and the rest, this is easy to port to the new format, as we only
need to identify untracked files and the rest is handled in the "else"
case.

untracked files are indicated by only a single question mark instead of
two question marks, so the conversion is easy.

Signed-off-by: Stefan Beller 
---
 submodule.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/submodule.c b/submodule.c
index 0c43f9f2b1..256f15fde1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1060,7 +1060,7 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
}
strbuf_reset();
 
-   argv_array_pushl(, "status", "--porcelain", NULL);
+   argv_array_pushl(, "status", "--porcelain=2", NULL);
if (ignore_untracked)
argv_array_push(, "-uno");
 
@@ -1070,11 +1070,12 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
cp.out = -1;
cp.dir = path;
if (start_command())
-   die("Could not run 'git status --porcelain' in submodule %s", 
path);
+   die("Could not run 'git status --porcelain=2' in submodule %s", 
path);
 
fp = xfdopen(cp.out, "r");
while (strbuf_getwholeline(, fp, '\n') != EOF) {
-   if ((buf.buf[0] == '?') && (buf.buf[1] == '?'))
+   /* regular untracked files */
+   if (buf.buf[0] == '?')
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
else
dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
@@ -1093,7 +1094,7 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
fclose(fp);
 
if (finish_command() && !ignore_cp_exit_code)
-   die("'git status --porcelain' failed in submodule %s", path);
+   die("'git status --porcelain=2' failed in submodule %s", path);
 
strbuf_release();
return dirty_submodule;
-- 
2.12.1.437.g2b7623d507

[PATCH 6/7] short status: improve reporting for submodule changes

[Stefan Beller]

If I add an untracked file to a submodule or modify a tracked file,
currently "git status --short" treats the change in the same way as
changes to the current HEAD of the submodule:

$ git clone --quiet --recurse-submodules 
https://gerrit.googlesource.com/gerrit
$ echo hello >gerrit/plugins/replication/stray-file
$ sed -i -e 's/.*//' gerrit/plugins/replication/.mailmap
$ git -C gerrit status --short
 M plugins/replication

This is by analogy with ordinary files, where "M" represents a change
that has not been added yet to the index.  But this change cannot be
added to the index without entering the submodule, "git add"-ing it,
and running "git commit", so the analogy is counterproductive.

Introduce new status letters " ?" and " m" for this.  These are similar
to the existing "??" and " M" but mean that the submodule (not the
parent project) has new untracked files and modified files, respectively.
The user can use "git add" and "git commit" from within the submodule to
add them.

Changes to the submodule's HEAD commit can be recorded in the index with
a plain "git add -u" and are shown with " M", like today.

To avoid excessive clutter, show at most one of " ?", " m", and " M" for
the submodule.  They represent increasing levels of change --- the last
one that applies is shown (e.g., " m" if there are both modified files
and untracked files in the submodule, or " M" if the submodule's HEAD
has been modified and it has untracked files).

While making these changes, we need to make sure to not break porcelain
level 1, which shares code with "status --short".  We only change
"git status --short".

Non-short "git status" and "git status --porcelain=2" already handle
these cases by showing more detail:

$ git -C gerrit status --porcelain=2
1 .M S.MU 16 16 16 305c864db28eb0c77c8499bc04c87de3f849cf3c 
305c864db28eb0c77c8499bc04c87de3f849cf3c plugins/replication
$ git -C gerrit status
[...]
modified:   plugins/replication (modified content, untracked content)

Scripts caring about these distinctions should use --porcelain=2.

Helped-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---
 Documentation/git-status.txt |  9 +++
 t/t3600-rm.sh| 18 ++
 t/t7506-status-submodule.sh  | 57 
 wt-status.c  | 13 --
 4 files changed, 90 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..01b457c322 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -181,6 +181,13 @@ in which case `XY` are `!!`.
 !   !ignored
 -
 
+Submodules have more state and instead report
+   Mthe submodule has a different HEAD than
+recorded in the index
+   mthe submodule has modified content
+   ?the submodule has untracked files
+
+
 If -b is used the short-format status is preceded by a line
 
 ## branchname tracking info
@@ -210,6 +217,8 @@ field from the first filename).  Third, filenames 
containing special
 characters are not specially formatted; no quoting or
 backslash-escaping is performed.
 
+Any submodule changes are reported as modified `M` instead of `m` or single 
`?`.
+
 Porcelain Format Version 2
 ~~
 
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 5aa6db584c..a6e5c5bd56 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -268,6 +268,14 @@ cat >expect.modified actual &&
@@ -436,7 +444,7 @@ test_expect_success 'rm of a populated submodule with 
untracked files fails unle
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified actual &&
+   test_cmp expect.modified_untracked actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
@@ -621,7 +629,7 @@ test_expect_success 'rm of a populated nested submodule 
with different nested HE
test -d submod &&
test -f submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
-   test_cmp expect.modified actual &&
+   test_cmp expect.modified_inside actual &&
git rm -f submod &&
test ! -d submod &&
git status -s -uno --ignore-submodules=none >actual &&
@@ -636,7 +644,7 @@ test_expect_success 'rm of a populated nested submodule 
with nested modification
test -d submod &&
test -f submod/.git &&

[PATCH 2/7] submodule.c: factor out early loop termination in is_submodule_modified

[Stefan Beller]

This makes it easier for a follow up patch.

Signed-off-by: Stefan Beller 
---
 submodule.c | 15 +++
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 2c667ac95a..e52cb8a958 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1075,16 +1075,15 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
len = strbuf_read(, cp.out, 1024);
line = buf.buf;
while (len > 2) {
-   if ((line[0] == '?') && (line[1] == '?')) {
+   if ((line[0] == '?') && (line[1] == '?'))
dirty_submodule |= DIRTY_SUBMODULE_UNTRACKED;
-   if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED)
-   break;
-   } else {
+   else
dirty_submodule |= DIRTY_SUBMODULE_MODIFIED;
-   if (ignore_untracked ||
-   (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED))
-   break;
-   }
+
+   if ((dirty_submodule & DIRTY_SUBMODULE_MODIFIED) &&
+   ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) || 
ignore_untracked))
+   break;
+
next_line = strchr(line, '\n');
if (!next_line)
break;
-- 
2.12.1.437.g2b7623d507

[PATCH v6 0/7] short status: improve reporting for submodule changes

[Stefan Beller]

v6:
* kill the child once we know all information that we ask for, as an 
optimization
* reordered the patches for that
* strbuf_getwholeline instead of its _fd version.

v5:
* fixed rebase error in the first 2 patches
* the last 3 patches introduce behavior change outside the scope of 
is_modified_submodule
  whereas the first 4 ought to just be local changes.
  
Thanks,
Stefan

v4:
* broken down in a lot of tiny patches.

Jonathan wrote:
> Patch 1/3 is the one that gives me pause, since I hadn't been able to
> chase down all callers.  Would it be feasible to split that into two:
> one patch to switch to --porcelain=2 but not change behavior, and a
> separate patch to improve what is stored in the dirty_submodule flags?

This part is in the latest patch now.

Thanks,
Stefan


v3:
This comes as a series; first I'd like to refactor is_submodule_modified
to take advantage of the new porcelain=2 plumbing switch to check for changes
in the submodule.

On top of the refactoring comes the actual change, which moved the
rewriting of the submodule change indicator letter to the collection part.

Thanks,
Stefan

Stefan Beller (8):
  submodule.c: port is_submodule_modified to use porcelain 2
  submodule.c: use argv_array in is_submodule_modified
  submodule.c: convert is_submodule_modified to use
strbuf_getwholeline_fd
  submodule.c: port is_submodule_modified to use porcelain 2
  submodule.c: factor out early loop termination in
is_submodule_modified
  submodule.c: stricter checking for submodules in is_submodule_modified
  short status: improve reporting for submodule changes
  submodule.c: correctly handle nested submodules in
is_submodule_modified

 Documentation/git-status.txt |  9 +++
 submodule.c  | 56 ---
 t/t3600-rm.sh| 18 ++
 t/t7506-status-submodule.sh  | 57 
 wt-status.c  | 13 --
 5 files changed, 116 insertions(+), 37 deletions(-)

-- 
2.12.1.438.gb674c4c09c

[PATCH 1/7] submodule.c: use argv_array in is_submodule_modified

[Stefan Beller]

struct argv_array is easier to use and maintain

Signed-off-by: Stefan Beller 
---
 submodule.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..2c667ac95a 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1043,12 +1043,6 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
 {
ssize_t len;
struct child_process cp = CHILD_PROCESS_INIT;
-   const char *argv[] = {
-   "status",
-   "--porcelain",
-   NULL,
-   NULL,
-   };
struct strbuf buf = STRBUF_INIT;
unsigned dirty_submodule = 0;
const char *line, *next_line;
@@ -1066,10 +1060,10 @@ unsigned is_submodule_modified(const char *path, int 
ignore_untracked)
}
strbuf_reset();
 
+   argv_array_pushl(, "status", "--porcelain", NULL);
if (ignore_untracked)
-   argv[2] = "-uno";
+   argv_array_push(, "-uno");
 
-   cp.argv = argv;
prepare_submodule_repo_env(_array);
cp.git_cmd = 1;
cp.no_stdin = 1;
-- 
2.12.1.437.g2b7623d507

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Fri, Mar 24, 2017 at 11:04:27AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > It seems like t7030 should just skip_all when the GPG prereq is not
> > met (it's not wrong to mark each test that's added, but it would have
> > made this particular mistake harder).
> 
> I'd leave that to be done by others after the dust settles ;-).
> 
> Here is what I have right now (proposed log message has updates to
> match rather obvious changes to the tests).

Thanks, this looks good to me.

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Jeff King]

On Fri, Mar 24, 2017 at 12:49:43PM -0400, Jeff King wrote:

> On Fri, Mar 24, 2017 at 09:45:30AM -0700, Junio C Hamano wrote:
> 
> > I actually think this uncovers another class of breakage.  t7030
> > tests should be protected with GPG prereq and 'fourth-signed' that
> > is made only with the prereq in the first test will not be found.
> 
> It seems like t7030 should just skip_all when the GPG prereq is not
> met (it's not wrong to mark each test that's added, but it would have
> made this particular mistake harder).
> 
> > t7004 probably has the same issue.
> 
> These ones should be marked individually, though.

I started to prepare a patch for t7030, but given that we want to do
this all in one patch anyway (for bisectability), I think it probably
makes sense to just add the missing GPG prereqs as part of the patch you
posted. If somebody wants to convert t7030 to skip_all on top that's
fine, but it's not that big a deal.

-Peff

Re: [PATCH 3/3] t7004, t7030: fix here-doc syntax errors

[Junio C Hamano]

Jeff King  writes:

> It seems like t7030 should just skip_all when the GPG prereq is not
> met (it's not wrong to mark each test that's added, but it would have
> made this particular mistake harder).

I'd leave that to be done by others after the dust settles ;-).  

Here is what I have right now (proposed log message has updates to
match rather obvious changes to the tests).

-- >8 --
From: Santiago Torres 
Date: Thu, 23 Mar 2017 18:28:47 -0400
Subject: [PATCH] t7004, t7030: fix here-doc syntax errors

Jan Palus noticed that some here-doc are spelled incorrectly,
resulting the entire remainder of the test snippet being slurped
into the "expect" file as if it were data, e.g. in this sequence

cat >expect 
Helped-by: Jeff King 
Signed-off-by: Santiago Torres 
Signed-off-by: Junio C Hamano 
---
 t/t7004-tag.sh| 12 +---
 t/t7030-verify-tag.sh | 12 +---
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index b53a2e5e41..f67bbb8abc 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -847,18 +847,16 @@ test_expect_success GPG 'verifying a forged tag should 
fail' '
test_must_fail git tag -v forged-tag
 '
 
-test_expect_success 'verifying a proper tag with --format pass and format 
accordingly' '
-   cat >expect <<-\EOF
+test_expect_success GPG 'verifying a proper tag with --format pass and format 
accordingly' '
+   cat >expect <<-\EOF &&
tagname : signed-tag
-   EOF &&
+   EOF
git tag -v --format="tagname : %(tag)" "signed-tag" >actual &&
test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
-   tagname : forged-tag
-   EOF &&
+test_expect_success GPG 'verifying a forged tag with --format should fail 
silently' '
+   >expect &&
test_must_fail git tag -v --format="tagname : %(tag)" "forged-tag" 
>actual &&
test_cmp expect actual
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index d62ccbb98e..b4b49eeb08 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -125,18 +125,16 @@ test_expect_success GPG 'verify multiple tags' '
test_cmp expect.stderr actual.stderr
 '
 
-test_expect_success 'verifying tag with --format' '
-   cat >expect <<-\EOF
+test_expect_success GPG 'verifying tag with --format' '
+   cat >expect <<-\EOF &&
tagname : fourth-signed
-   EOF &&
+   EOF
git verify-tag --format="tagname : %(tag)" "fourth-signed" >actual &&
test_cmp expect actual
 '
 
-test_expect_success 'verifying a forged tag with --format fail and format 
accordingly' '
-   cat >expect <<-\EOF
-   tagname : 7th forged-signed
-   EOF &&
+test_expect_success GPG 'verifying a forged tag with --format should fail 
silently' '
+   >expect &&
test_must_fail git verify-tag --format="tagname : %(tag)" $(cat 
forged1.tag) >actual-forged &&
test_cmp expect actual-forged
 '
-- 
2.12.1-432-gf364f02724

Re: [PATCH v3 2/2] l10n: Add git-add.txt to localized man pages

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> On Mon, Mar 20, 2017 at 11:05 PM, Junio C Hamano  wrote:
>> But more importantly, aren't we essentially adding an equivalent of
>>
>> cd Documentation && cat git-*.txt
>>
>> to our codebase?
>>
>> Surely we cannot avoid having a copy of all messages that are to be
>> translated using msgid/msgstr based approach, and we already do so
>> for end-user-facing in-program strings, but it just feels a bit too
>> much having to carry a duplicate (and slightly a stale) copy of the
>> entire documentation set around.  For N languages, we'll have an
>> equivalent for N copies of the English text, in addition to the
>> translated documentation.
>
> As someone reading this thread from the sidelines you never elaborate
> on why this is a problem worth solving (other than "a bit too much")
> before everyone downthread jumped on trying to figure out how to solve
> this out-of tree somehow.

I do not particularly see the size as an issue that must be solved;
to me, it is "nice to solve".

But going back and finding this from Jean-Noel in an earlier
message:

... This is one of the points raised in the first RFC mail. Splitting this
part would help a lot manage the translations with their own workflow,
would not clutter the main repo with files not really needed for
packaging and would simplify dealing with the interaction with crowd
translation websites which can directly push translation content to a
git repo.

there may be other benefits we may be able to reap from such a
split.

Re: [PATCH] read-cache: call verify_hdr() in a background thread

[Jonathan Nieder]

Jeff Hostetler wrote:
> On 3/24/2017 12:35 PM, Jonathan Nieder wrote:

>> What happens if there is an error before the code reaches the end of
>> the function?  I think there needs to be a verify_hdr_finish call in
>> the 'unmap:' cleanup section.
>
> But the "unmap" section calls die().  Do need to join first ??
> (It's OK if we do, just asking.)

Good point.  Now I wonder why the 'unmap:' section exists at all.

$ git log -Sunmap: read-cache.c 
commit e83c5163316f89bfbde7d9ab23ca2e25604af290
Author: Linus Torvalds 
Date:   Thu Apr 7 15:13:13 2005 -0700

Initial revision of "git", the information manager from hell

So the unmap: section has been there since day one.  At that point
it used 'return error(...)', so it made more sense.  Where did the
die() come from, then?

commit 5d1a5c02e8ac1c16688ea4a44512245f25a49f8a
Author: Linus Torvalds 
Date:   Sat Oct 1 13:24:27 2005 -0700

[PATCH] Better error reporting for "git status"

I think it should be safe to eliminate the 'unmap' section altogether
and that that change just forgot to, but in any case it's orthogonal
to your patch.

Thanks and sorry for the confusion,
Jonathan

Re: [PATCH v2 0/7] thread lazy_init_name_hash

[Junio C Hamano]

Jeff Hostetler  writes:

> WRT the assert() in name-hash.c, Stefan suggested converting it
> to an if-!-die form in an earlier message in this thread.  I'm OK
> with that or with removing the assert completely.

I actually am OK with leaving things as they are ;-)

Re: [PATCH] name-hash: add test-lazy-init-name-hash to .gitignore

[Ramsay Jones]

On 24/03/17 17:26, Ramsay Jones wrote:
> 
> Signed-off-by: Ramsay Jones 
> ---
> 
> Hi Jeff,
> 
> If you need to re-roll your 'jh/memihash-opt' branch, could you please
> squash this into the relevant patch (commit f25dde4fbf, "name-hash: add
> test-lazy-init-name-hash", 23-03-2017).
> 
> Thanks!
> 
> ATB,
> Ramsay Jones
> 
>  t/helper/.gitignore | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/t/helper/.gitignore b/t/helper/.gitignore
> index 5f68aa8f8..57bdd4b8e 100644
> --- a/t/helper/.gitignore
> +++ b/t/helper/.gitignore
> @@ -12,6 +12,7 @@
>  /test-hashmap
>  /test-index-version
>  /test-line-buffer
> +/test-lazy-init-name-hash

Heh, you could even put it before the '/test-line-buffer' entry! ;-)

Ahem.

ATB,
Ramsay Jones

>  /test-match-trees
>  /test-mergesort
>  /test-mktemp
> 

Re: [PATCH v2] log: if --decorate is not given, default to --decorate=auto

[Junio C Hamano]

Alex Henrie  writes:

> +test_expect_success TTY 'log output on a TTY' '
> + git log --oneline --decorate >expect.short &&
> +
> + test_terminal git log --oneline >actual &&
> + test_cmp expect.short actual
> +'
> +

Nice.  I didn't know test_terminal was so easy to use ;-)

Looks good.  Will queue.

Thanks.

Re: [PATCH] read-cache: call verify_hdr() in a background thread

[Jeff Hostetler]

On 3/24/2017 12:35 PM, Jonathan Nieder wrote:

g...@jeffhostetler.com wrote:


From: Jeff Hostetler 

Teash do_read_index() in read-cache.c to call verify_hdr()

...
Nice.  Do you have example commands I can run to reproduce
that benchmark?  (Even better if you can phrase that as a
patch against t/perf/.)


I debated doing a t/perf and/or t/helper to demonstrate this
like I did for the lazy-init-name-hash changes the other day,
but decided against it.  I'll put together something and
include it in the next version.



[...]

--- a/read-cache.c
+++ b/read-cache.c
@@ -1564,6 +1564,83 @@ static void post_read_index_from(struct index_state 
*istate)
tweak_untracked_cache(istate);
 }

+#ifdef NO_PTHREADS
+
+struct verify_hdr_thread_data {
+   struct cache_header *hdr;
+   size_t size;


'size' appears to always be cast to an unsigned long when it's
used.  Why not use unsigned long consistently?


+};
+
+/*
+ * Non-threaded version does all the work immediately.
+ * Returns < 0 on error or bad signature.
+ */
+static int verify_hdr_start(struct verify_hdr_thread_data *d)
+{
+   return verify_hdr(d->hdr, (unsigned long)d->size);
+}
+
+static int verify_hdr_finish(struct verify_hdr_thread_data *d)
+{
+   return 0;
+}
+
+#else
+
+#include 


Please put this at the top of the file with other #includes.  One
simple way to do that is to #include "thread-utils.h" at the top of
the file unconditionally.


+
+/*
+ * Require index file to be larger than this threshold before
+ * we bother using a background thread to verify the SHA.
+ */
+#define VERIFY_HDR_THRESHOLD(1024)


nits: (1) no need for parens for a numerical macro like this
(2) comment can be made briefer and more explicit:

/*
 * Index size threshold in bytes before it's worth bothering to
 * use a background thread to verify the index file.
 */

How was this value chosen?


This was somewhat at random.  I'll update with the t/perf stuff.




+
+struct verify_hdr_thread_data {
+   pthread_t thread_id;
+   struct cache_header *hdr;
+   size_t size;
+   int result;
+};


All structs are data.  Other parts of git seem to name this kind of
callback cookie *_cb_data, so perhaps verify_hdr_cb_data?

On the other hand this seems to also be used by the caller as a handle
to the async verify_hdr process. Maybe verify_hdr_state?

This seems to be doing something similar to the existing 'struct
async' interface.  Could it use that instead, or does it incur too
much overhead?  An advantage would be avoiding having to handle the
NO_PTHREADS ifdef-ery.


+
+/*
+ * Thread proc to run verify_hdr() computation in a background thread.
+ */
+static void *verify_hdr_thread_proc(void *_data)


Please don't name identifiers with a leading underscore --- those are
reserved names.


+{
+   struct verify_hdr_thread_data *d = _data;
+   d->result = verify_hdr(d->hdr, (unsigned long)d->size);
+   return NULL;
+}


I was just modeling the code on what I saw in preload-index.c.
There the #ifdef side defines the trivial functions. Then the #else
and the #include, the struct thread_data, and then the
preload_thread(void *_data) function declaration.  But blame reports
that code dating back to 2008.  Is there an example of a newer
preferred style somewhere ?


+
+/*
+ * Threaded version starts background thread and returns zero
+ * to indicate that we don't know the hash is bad yet.  If the
+ * index is too small, we just do the work imediately.
+ */
+static int verify_hdr_start(struct verify_hdr_thread_data *d)


This comment restates what the code says.  Is there background or
something about the intent behind the code that could be said instead
to help the reader?  Otherwise I'd suggest removing the comment.

[...]

What happens if there is an error before the code reaches the end of
the function?  I think there needs to be a verify_hdr_finish call in
the 'unmap:' cleanup section.


But the "unmap" section calls die().  Do need to join first ??
(It's OK if we do, just asking.)



The rest looks reasonable.

Thanks and hope that helps,
Jonathan



Thanks,
Jeff

[PATCH 2/4] fast-import: use xsnprintf for formatting headers

[Jeff King]

The stream_blob() function checks the return value of
snprintf and dies. This is more simply done with
xsnprintf (and matches the similar call in store_object).

The message the user would get is less specific, but since
the point is that this _shouldn't_ ever happen, that's OK.

Signed-off-by: Jeff King 
---
 fast-import.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 4e0f3f5dd..4a057e81f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1237,9 +1237,7 @@ static void stream_blob(uintmax_t len, unsigned char 
*sha1out, uintmax_t mark)
sha1file_checkpoint(pack_file, );
offset = checkpoint.offset;
 
-   hdrlen = snprintf((char *)out_buf, out_sz, "blob %" PRIuMAX, len) + 1;
-   if (out_sz <= hdrlen)
-   die("impossibly large object header");
+   hdrlen = xsnprintf((char *)out_buf, out_sz, "blob %" PRIuMAX, len) + 1;
 
git_SHA1_Init();
git_SHA1_Update(, out_buf, hdrlen);
-- 
2.12.1.843.g1937c56c2

[PATCH 4/4] pack.h: define largest possible encoded object size

[Jeff King]

Several callers use fixed buffers for storing the pack
object header, and they've picked 10 as a magic number. This
is reasonable, since it handles objects up to 2^67. But
let's give them a constant so it's clear that the number
isn't pulled out of thin air.

Signed-off-by: Jeff King 
---
 builtin/pack-objects.c | 6 --
 pack.h | 6 ++
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index cf3fc50a2..84af7c232 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -239,7 +239,8 @@ static unsigned long write_no_reuse_object(struct sha1file 
*f, struct object_ent
   unsigned long limit, int 
usable_delta)
 {
unsigned long size, datalen;
-   unsigned char header[10], dheader[10];
+   unsigned char header[MAX_PACK_OBJECT_HEADER],
+ dheader[MAX_PACK_OBJECT_HEADER];
unsigned hdrlen;
enum object_type type;
void *buf;
@@ -353,7 +354,8 @@ static off_t write_reuse_object(struct sha1file *f, struct 
object_entry *entry,
off_t offset;
enum object_type type = entry->type;
off_t datalen;
-   unsigned char header[10], dheader[10];
+   unsigned char header[MAX_PACK_OBJECT_HEADER],
+ dheader[MAX_PACK_OBJECT_HEADER];
unsigned hdrlen;
 
if (entry->delta)
diff --git a/pack.h b/pack.h
index 87e456d5e..5c2158746 100644
--- a/pack.h
+++ b/pack.h
@@ -84,6 +84,12 @@ extern int verify_pack(struct packed_git *, verify_fn fn, 
struct progress *, uin
 extern off_t write_pack_header(struct sha1file *f, uint32_t);
 extern void fixup_pack_header_footer(int, unsigned char *, const char *, 
uint32_t, unsigned char *, off_t);
 extern char *index_pack_lockfile(int fd);
+
+/*
+ * The "hdr" output buffer should be at least this big, which will handle sizes
+ * up to 2^67.
+ */
+#define MAX_PACK_OBJECT_HEADER 10
 extern int encode_in_pack_object_header(unsigned char *hdr, int hdr_len,
enum object_type, uintmax_t);
 
-- 
2.12.1.843.g1937c56c2

[PATCH] name-hash: add test-lazy-init-name-hash to .gitignore

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Jeff,

If you need to re-roll your 'jh/memihash-opt' branch, could you please
squash this into the relevant patch (commit f25dde4fbf, "name-hash: add
test-lazy-init-name-hash", 23-03-2017).

Thanks!

ATB,
Ramsay Jones

 t/helper/.gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index 5f68aa8f8..57bdd4b8e 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -12,6 +12,7 @@
 /test-hashmap
 /test-index-version
 /test-line-buffer
+/test-lazy-init-name-hash
 /test-match-trees
 /test-mergesort
 /test-mktemp
-- 
2.12.0

[PATCH 3/4] encode_in_pack_object_header: respect output buffer length

[Jeff King]

The encode_in_pack_object_header() writes a variable-length
header to an output buffer, but it doesn't actually know
long the buffer is. At first glance, this looks like it
might be possible to overflow.

In practice, this is probably impossible. The smallest
buffer we use is 10 bytes, which would hold the header for
an object up to 2^67 bytes. Obviously we're not likely to
see such an object, but we might worry that an object could
lie about its size (causing us to overflow before we realize
it does not actually have that many bytes). But the argument
is passed as a uintmax_t. Even on systems that have __int128
available, uintmax_t is typically restricted to 64-bit by
the ABI.

So it's unlikely that a system exists where this could be
exploited. Still, it's easy enough to use a normal out/len
pair and make sure we don't write too far. That protects the
hypothetical 128-bit system, makes it harder for callers to
accidentally specify a too-small buffer, and makes the
resulting code easier to audit.

Note that the one caller in fast-import tried to catch such
a case, but did so _after_ the call (at which point we'd
have already overflowed!). This check can now go away.

Signed-off-by: Jeff King 
---
 builtin/pack-objects.c |  6 --
 bulk-checkin.c |  2 +-
 fast-import.c  | 10 +-
 pack-write.c   |  5 -
 pack.h |  3 ++-
 5 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 16517f263..cf3fc50a2 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -286,7 +286,8 @@ static unsigned long write_no_reuse_object(struct sha1file 
*f, struct object_ent
 * The object header is a byte of 'type' followed by zero or
 * more bytes of length.
 */
-   hdrlen = encode_in_pack_object_header(type, size, header);
+   hdrlen = encode_in_pack_object_header(header, sizeof(header),
+ type, size);
 
if (type == OBJ_OFS_DELTA) {
/*
@@ -358,7 +359,8 @@ static off_t write_reuse_object(struct sha1file *f, struct 
object_entry *entry,
if (entry->delta)
type = (allow_ofs_delta && entry->delta->idx.offset) ?
OBJ_OFS_DELTA : OBJ_REF_DELTA;
-   hdrlen = encode_in_pack_object_header(type, entry->size, header);
+   hdrlen = encode_in_pack_object_header(header, sizeof(header),
+ type, entry->size);
 
offset = entry->in_pack_offset;
revidx = find_pack_revindex(p, offset);
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 991b4a13e..ddb6070c4 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -105,7 +105,7 @@ static int stream_to_pack(struct bulk_checkin_state *state,
 
git_deflate_init(, pack_compression_level);
 
-   hdrlen = encode_in_pack_object_header(type, size, obuf);
+   hdrlen = encode_in_pack_object_header(obuf, sizeof(obuf), type, size);
s.next_out = obuf + hdrlen;
s.avail_out = sizeof(obuf) - hdrlen;
 
diff --git a/fast-import.c b/fast-import.c
index 4a057e81f..f6f416f20 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1173,7 +1173,8 @@ static int store_object(
delta_count_by_type[type]++;
e->depth = last->depth + 1;
 
-   hdrlen = encode_in_pack_object_header(OBJ_OFS_DELTA, deltalen, 
hdr);
+   hdrlen = encode_in_pack_object_header(hdr, sizeof(hdr),
+ OBJ_OFS_DELTA, deltalen);
sha1write(pack_file, hdr, hdrlen);
pack_size += hdrlen;
 
@@ -1184,7 +1185,8 @@ static int store_object(
pack_size += sizeof(hdr) - pos;
} else {
e->depth = 0;
-   hdrlen = encode_in_pack_object_header(type, dat->len, hdr);
+   hdrlen = encode_in_pack_object_header(hdr, sizeof(hdr),
+ type, dat->len);
sha1write(pack_file, hdr, hdrlen);
pack_size += hdrlen;
}
@@ -1246,9 +1248,7 @@ static void stream_blob(uintmax_t len, unsigned char 
*sha1out, uintmax_t mark)
 
git_deflate_init(, pack_compression_level);
 
-   hdrlen = encode_in_pack_object_header(OBJ_BLOB, len, out_buf);
-   if (out_sz <= hdrlen)
-   die("impossibly large object header");
+   hdrlen = encode_in_pack_object_header(out_buf, out_sz, OBJ_BLOB, len);
 
s.next_out = out_buf + hdrlen;
s.avail_out = out_sz - hdrlen;
diff --git a/pack-write.c b/pack-write.c
index 88bc7f9f7..c057513f1 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -304,7 +304,8 @@ char *index_pack_lockfile(int ip_out)
  *  - each byte afterwards: low seven bits are size continuation,
  *with the high bit being "size continues"
  */
-int encode_in_pack_object_header(enum object_type type, uintmax_t 

[PATCH 1/4] fast-import: use xsnprintf for writing sha1s

[Jeff King]

When we have to write a sha1 with a newline, we do so by
copying both into a single buffer, so that we can issue a
single write() call.

We use snprintf but don't bother to check the output, since
we know it will fit. However, we should use xsnprintf() in
such a case so that we're notified if our assumption turns
out to be wrong (and to make it easier to audit for
unchecked snprintf calls).

Signed-off-by: Jeff King 
---
This is ready for conversion to oid_to_hex, too, but I avoided that
here. The buffer would need to be allocated with the new GIT_MAX_HEXSZ,
which is not yet available. So I figured it was better to leave it than
half-convert it and leave brian wondering whether it's really supposed
to be GIT_MAX_HEXSZ or GIT_SHA1_HEXSZ.

 fast-import.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index 41a539f97..4e0f3f5dd 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3003,7 +3003,7 @@ static void parse_get_mark(const char *p)
if (!oe)
die("Unknown mark: %s", command_buf.buf);
 
-   snprintf(output, sizeof(output), "%s\n", sha1_to_hex(oe->idx.sha1));
+   xsnprintf(output, sizeof(output), "%s\n", sha1_to_hex(oe->idx.sha1));
cat_blob_write(output, 41);
 }
 
-- 
2.12.1.843.g1937c56c2

[PATCH 0/4] a few minor buffer cleanups in fast-import

[Jeff King]

I don't think any of these is a triggerable bug. They're just cleanups
to make it more obvious that the code is doing the right thing (and
making it harder to do the wrong thing).

  [1/4]: fast-import: use xsnprintf for writing sha1s
  [2/4]: fast-import: use xsnprintf for formatting headers
  [3/4]: encode_in_pack_object_header: respect output buffer length
  [4/4]: pack.h: define largest possible encoded object size

 builtin/pack-objects.c | 12 
 bulk-checkin.c |  2 +-
 fast-import.c  | 16 +++-
 pack-write.c   |  5 -
 pack.h |  9 -
 5 files changed, 28 insertions(+), 16 deletions(-)

-Peff

[PATCH] push: allow atomic flag via configuration

[Romuald Brunet]

Added a "push.atomic" option to git-config to allow site-specific
configuration of the atomic flag of git push

Signed-off-by: Romuald Brunet 
---
 Documentation/config.txt   |  5 +
 builtin/push.c |  6 ++
 contrib/completion/git-completion.bash |  1 +
 t/t5543-atomic-push.sh | 31 +++
 4 files changed, 43 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0d8df5a9f..c826c86ae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2555,6 +2555,11 @@ new default).
 
 --
 
+push.atomic::
+   If set to true enable `--atomic` option by default.  You
+   may override this configuration at time of push by specifying
+   `--no-atomic`.
+
 push.followTags::
If set to true enable `--follow-tags` option by default.  You
may override this configuration at time of push by specifying
diff --git a/builtin/push.c b/builtin/push.c
index 5c22e9f2e..03066f3f7 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -498,6 +498,12 @@ static int git_push_config(const char *k, const char *v, 
void *cb)
const char *value;
if (!git_config_get_value("push.recursesubmodules", ))
recurse_submodules = 
parse_push_recurse_submodules_arg(k, value);
+   } else if (!strcmp(k, "push.atomic")) {
+   if (git_config_bool(k, v))
+   *flags |= TRANSPORT_PUSH_ATOMIC;
+   else
+   *flags &= ~TRANSPORT_PUSH_ATOMIC;
+   return 0;
}
 
return git_default_config(k, v, NULL);
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index fc32286a4..8d38f5f8f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2396,6 +2396,7 @@ _git_config ()
pull.octopus
pull.twohead
push.default
+   push.atomic
push.followTags
rebase.autosquash
rebase.stat
diff --git a/t/t5543-atomic-push.sh b/t/t5543-atomic-push.sh
index 3480b3300..7c573b85b 100755
--- a/t/t5543-atomic-push.sh
+++ b/t/t5543-atomic-push.sh
@@ -191,4 +191,35 @@ test_expect_success 'atomic push is not advertised if 
configured' '
test_refs master HEAD@{1}
 '
 
+test_expect_success 'atomic option possible via git-config' '
+   # prepare the repo
+   mk_repo_pair &&
+   (
+   cd workbench &&
+   test_commit one &&
+   git checkout -b second master &&
+   test_commit two &&
+   git push --mirror up
+   ) &&
+   # a third party modifies the server side:
+   (
+   cd upstream &&
+   git checkout second &&
+   git tag test_tag second
+   ) &&
+   # see if we can now push both branches.
+   (
+   cd workbench &&
+   git config push.atomic true &&
+   git checkout master &&
+   test_commit three &&
+   git checkout second &&
+   test_commit four &&
+   git tag test_tag &&
+   test_must_fail git push --tags up master second
+   ) &&
+   test_refs master HEAD@{3} &&
+   test_refs second HEAD@{1}
+'
+
 test_done
-- 
2.12.0.dirty

Re: [PATCH v1 2/3] sub-process: refactor the filter process code into a reusable module

[Junio C Hamano]

Junio C Hamano  writes:

> ...
> And for something of sub-process.[ch]'s size, I suspect that it
> would have more than 1 such logical unit to be independently
> refactored, so in total, I would suspect the series would become
>
> 1 (boring mechanical part) +
> 2 or more (refactoring) +
> 1 (final movement)
>
> i.e. 4 or more patches?

To avoid confusion (although readers may not require), even though I
explained "boring mechanical part" first and "refactoring", that was
purely for explanation.  

In practice, I would expect that it would be easier to both do and
review if refactoring is done with the original name.  

A function that will keep its name in the final result (e.g.
start_multi_file_filter()) will call a new and more generic helper
function.  The new helper may start using the new name from the
get-go (e.g. subprocess_start()), but the data types it shares with
the original part of the code (e.g. 'struct cmd2process') may still
be using the original name.

And after completing "2 or more" refactoring would be a good place
to do the remaining "boring mechanical rename".  IOW, the count
above could be

 2 or more (refactoring) +
 1 (boring mechanical part) +
 1 (final movement)

and I didn't mean to say that you need to rename first.  What we
want is "if you need to have a single large patch that cannot be
split, see if you can make it purely mechanical.", as a single large
patch that is _not_ mechanical conversion is the worst kind of patch
for reviewers.