[PATCH v4 1/4] t7501: add coverage for flags which imply dry runs

[Samuel Lijin]

The behavior of git commit, when doing a dry run, changes if there are
unresolved/resolved merge conflicts, but the test suite currently only
asserts that `git commit --dry-run` succeeds when all merge conflicts
are resolved.

Add tests to document the behavior of all flags (i.e. `--dry-run`,
`--short`, `--porcelain`, and `--long`) which imply a dry run when (1)
there are only unresolved merge conflicts, (2) when there are both
unresolved and resolved merge conflicts, and (3) when all merge
conflicts are resolved.

When testing behavior involving resolved merge conflicts, resolve merge
conflicts by replacing each merge conflict with completely new contents,
rather than choosing the contents associated with one of the parent
commits, since the latter decision has no bearing on the behavior of a
dry run commit invocation.

Verify that a dry run invocation of git commit does not create a new
commit by asserting that HEAD has not changed, instead of by crafting
the commit.

Signed-off-by: Samuel Lijin 
---
 t/t7501-commit.sh | 146 +-
 1 file changed, 132 insertions(+), 14 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 9dbbd01fc..e49dfd0a2 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -664,24 +664,142 @@ test_expect_success '--only works on to-be-born branch' '
test_cmp expected actual
 '
 
-test_expect_success '--dry-run with conflicts fixed from a merge' '
-   # setup two branches with conflicting information
-   # in the same file, resolve the conflict,
-   # call commit with --dry-run
-   echo "Initial contents, unimportant" >test-file &&
-   git add test-file &&
+test_expect_success 'prepare commits that can be used to trigger a merge 
conflict' '
+   # setup two branches with conflicting contents in two paths
+   echo "Initial contents, unimportant" | tee test-file1 test-file2 &&
+   git add test-file1 test-file2 &&
git commit -m "Initial commit" &&
-   echo "commit-1-state" >test-file &&
-   git commit -m "commit 1" -i test-file &&
+   echo "commit-1-state" | tee test-file1 test-file2 &&
+   git commit -m "commit 1" -i test-file1 test-file2 &&
git tag commit-1 &&
git checkout -b branch-2 HEAD^1 &&
-   echo "commit-2-state" >test-file &&
-   git commit -m "commit 2" -i test-file &&
-   ! $(git merge --no-commit commit-1) &&
-   echo "commit-2-state" >test-file &&
-   git add test-file &&
+   echo "commit-2-state" | tee test-file1 test-file2 &&
+   git commit -m "commit 2" -i test-file1 test-file2 &&
+   git tag commit-2
+'
+
+test_expect_success '--dry-run with only unresolved merge conflicts' '
+   git reset --hard commit-2 &&
+   test_must_fail git merge --no-commit commit-1 &&
+   test_must_fail git commit --dry-run &&
+   git rev-parse commit-2 >expected &&
+   git rev-parse HEAD >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success '--short with only unresolved merge conflicts' '
+   git reset --hard commit-2 &&
+   test_must_fail git merge --no-commit commit-1 &&
+   test_must_fail git commit --short &&
+   git rev-parse commit-2 >expected &&
+   git rev-parse HEAD >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success '--porcelain with only unresolved merge conflicts' '
+   git reset --hard commit-2 &&
+   test_must_fail git merge --no-commit commit-1 &&
+   test_must_fail git commit --porcelain &&
+   git rev-parse commit-2 >expected &&
+   git rev-parse HEAD >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success '--long with only unresolved merge conflicts' '
+   git reset --hard commit-2 &&
+   test_must_fail git merge --no-commit commit-1 &&
+   test_must_fail git commit --long &&
+   git rev-parse commit-2 >expected &&
+   git rev-parse HEAD >actual &&
+   test_cmp expected actual
+'
+
+test_expect_failure '--dry-run with resolved and unresolved merge conflicts' '
+   git reset --hard commit-2 &&
+   test_must_fail git merge --no-commit commit-1 &&
+   echo "resolve one merge conflict" >test-file1 &&
+   git add test-file1 &&
+   test_must_fail git commit --dry-run &&
+   git rev-parse commit-2 >expected &&
+   git rev-parse HEAD >actual &&
+   test_cmp expected actual
+'
+
+test_expect_success '--short with resolv

[PATCH v4 0/4] Rerolling patch series to fix t7501

[Samuel Lijin]

Following up on Junio's review from last time.

Samuel Lijin (4):
  t7501: add coverage for flags which imply dry runs
  wt-status: rename commitable to committable
  wt-status: teach wt_status_collect about merges in progress
  commit: fix exit code when doing a dry run

 builtin/commit.c  |  32 +++---
 ref-filter.c  |   3 +-
 t/t7501-commit.sh | 150 ---
 wt-status.c   | 258 --
 wt-status.h   |  13 +--
 5 files changed, 298 insertions(+), 158 deletions(-)

-- 
2.18.0

[PATCH v4 2/4] wt-status: rename commitable to committable

[Samuel Lijin]

Fix a typo in the name of the committable bit in wt_status_state.

Signed-off-by: Samuel Lijin 
---
 builtin/commit.c | 18 +-
 wt-status.c  | 10 +-
 wt-status.h  |  2 +-
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 158e3f843..32f9db33b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -507,7 +507,7 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
wt_status_collect(s);
wt_status_print(s);
 
-   return s->commitable;
+   return s->committable;
 }
 
 static int is_a_merge(const struct commit *current_head)
@@ -653,7 +653,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
 {
struct stat statbuf;
struct strbuf committer_ident = STRBUF_INIT;
-   int commitable;
+   int committable;
struct strbuf sb = STRBUF_INIT;
const char *hook_arg1 = NULL;
const char *hook_arg2 = NULL;
@@ -870,7 +870,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
 
saved_color_setting = s->use_color;
s->use_color = 0;
-   commitable = run_status(s->fp, index_file, prefix, 1, s);
+   committable = run_status(s->fp, index_file, prefix, 1, s);
s->use_color = saved_color_setting;
} else {
struct object_id oid;
@@ -888,7 +888,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
for (i = 0; i < active_nr; i++)
if (ce_intent_to_add(active_cache[i]))
ita_nr++;
-   commitable = active_nr - ita_nr > 0;
+   committable = active_nr - ita_nr > 0;
} else {
/*
 * Unless the user did explicitly request a submodule
@@ -904,7 +904,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
if (ignore_submodule_arg &&
!strcmp(ignore_submodule_arg, "all"))
flags.ignore_submodules = 1;
-   commitable = index_differs_from(parent, , 1);
+   committable = index_differs_from(parent, , 1);
}
}
strbuf_release(_ident);
@@ -916,7 +916,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
 * explicit --allow-empty. In the cherry-pick case, it may be
 * empty due to conflict resolution, which the user should okay.
 */
-   if (!commitable && whence != FROM_MERGE && !allow_empty &&
+   if (!committable && whence != FROM_MERGE && !allow_empty &&
!(amend && is_a_merge(current_head))) {
s->display_comment_prefix = old_display_comment_prefix;
run_status(stdout, index_file, prefix, 0, s);
@@ -1186,14 +1186,14 @@ static int parse_and_validate_options(int argc, const 
char *argv[],
 static int dry_run_commit(int argc, const char **argv, const char *prefix,
  const struct commit *current_head, struct wt_status 
*s)
 {
-   int commitable;
+   int committable;
const char *index_file;
 
index_file = prepare_index(argc, argv, prefix, current_head, 1);
-   commitable = run_status(stdout, index_file, prefix, 0, s);
+   committable = run_status(stdout, index_file, prefix, 0, s);
rollback_index_files();
 
-   return commitable ? 0 : 1;
+   return committable ? 0 : 1;
 }
 
 define_list_config_array_extra(color_status_slots, {"added"});
diff --git a/wt-status.c b/wt-status.c
index 8827a256d..18ea333a5 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -773,7 +773,7 @@ static void wt_longstatus_print_updated(struct wt_status *s)
continue;
if (!shown_header) {
wt_longstatus_print_cached_header(s);
-   s->commitable = 1;
+   s->committable = 1;
shown_header = 1;
}
wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
@@ -1008,7 +1008,7 @@ static void wt_longstatus_print_verbose(struct wt_status 
*s)
rev.diffopt.use_color = 0;
wt_status_add_cut_line(s->fp);
}
-   if (s->verbose > 1 && s->commitable) {
+   if (s->verbose > 1 && s->committable) {
/* print_updated() printed a header, so do we */
if (s->fp != stdout)
wt_longstatus_print_trailer(s);
@@ -1089,7 +1089,7 @@ static void show_merge_in_progress(struct wt_status *s,
   

[PATCH v4 4/4] commit: fix exit code when doing a dry run

[Samuel Lijin]

In wt-status.c, the s->committable bit is set only in the call tree of
wt_longstatus_print(), and it is not always set correctly. This means
that in normal cases, if there are changes to be committed, or if there
is a merge in progress and all conflicts have been resolved, `--dry-run`
and `--long` return the correct exit code but `--short` and
`--porcelain` do not, even though all four flags imply dry run.
Moreover, if there is a merge in progress and some but not all conflicts
have been resolved, `--short` and `--porcelain` only return the correct
exit code by coincidence (because the codepaths they follow never touch
the committable bit), whereas `--dry-run` and `--long` return the wrong
exit code.

Teach wt_status_collect() to set s->committable correctly (if a merge is
in progress, committable should be set iff there are no unmerged
changes; otherwise, committable should be set iff there are changes in
the index) so that all four flags which imply dry runs return the
correct exit code in the above described situations and mark the
documenting tests as fixed.

Use the index_status field in the wt_status_change_data structs in
has_unmerged() to determine whether or not there are unmerged paths,
instead of the stagemask field, to improve readability.

Also stop setting s->committable in wt_longstatus_print_updated() and
show_merge_in_progress(), and const-ify wt_status_state in the method
signatures in those callpaths.

Signed-off-by: Samuel Lijin 
---
 t/t7501-commit.sh | 12 +++
 wt-status.c   | 80 +--
 2 files changed, 55 insertions(+), 37 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index e49dfd0a2..6dba526e6 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -99,12 +99,12 @@ test_expect_success '--dry-run with stuff to commit returns 
ok' '
git commit -m next -a --dry-run
 '
 
-test_expect_failure '--short with stuff to commit returns ok' '
+test_expect_success '--short with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --short
 '
 
-test_expect_failure '--porcelain with stuff to commit returns ok' '
+test_expect_success '--porcelain with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --porcelain
 '
@@ -714,7 +714,7 @@ test_expect_success '--long with only unresolved merge 
conflicts' '
test_cmp expected actual
 '
 
-test_expect_failure '--dry-run with resolved and unresolved merge conflicts' '
+test_expect_success '--dry-run with resolved and unresolved merge conflicts' '
git reset --hard commit-2 &&
test_must_fail git merge --no-commit commit-1 &&
echo "resolve one merge conflict" >test-file1 &&
@@ -747,7 +747,7 @@ test_expect_success '--porcelain with resolved and 
unresolved merge conflicts' '
test_cmp expected actual
 '
 
-test_expect_failure '--long with resolved and unresolved merge conflicts' '
+test_expect_success '--long with resolved and unresolved merge conflicts' '
git reset --hard commit-2 &&
test_must_fail git merge --no-commit commit-1 &&
echo "resolve one merge conflict" >test-file1 &&
@@ -769,7 +769,7 @@ test_expect_success '--dry-run with only resolved merge 
conflicts' '
test_cmp expected actual
 '
 
-test_expect_failure '--short with only resolved merge conflicts' '
+test_expect_success '--short with only resolved merge conflicts' '
git reset --hard commit-2 &&
test_must_fail git merge --no-commit commit-1 &&
echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
@@ -780,7 +780,7 @@ test_expect_failure '--short with only resolved merge 
conflicts' '
test_cmp expected actual
 '
 
-test_expect_failure '--porcelain with only resolved merge conflicts' '
+test_expect_success '--porcelain with only resolved merge conflicts' '
git reset --hard commit-2 &&
test_must_fail git merge --no-commit commit-1 &&
echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
diff --git a/wt-status.c b/wt-status.c
index af83fae68..fc239f61c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -724,6 +724,38 @@ static void wt_status_collect_untracked(struct wt_status 
*s)
s->untracked_in_ms = (getnanotime() - t_begin) / 100;
 }
 
+static int has_unmerged(const struct wt_status *s)
+{
+   int i;
+
+   for (i = 0; i < s->change.nr; i++) {
+   struct wt_status_change_data *d = (s->change.items[i]).util;
+   if (d->index_status == DIFF_STATUS_UNMERGED)
+   return 1;
+   }
+   return 0;
+}
+
+static void wt_status_mark_committable(
+   struct wt_status *s, const struct wt_status_state 

[PATCH v4 3/4] wt-status: teach wt_status_collect about merges in progress

[Samuel Lijin]

To fix the breakages documented by t7501, the next patch in this series
will teach wt_status_collect() how to set the committable bit, instead
of having wt_longstatus_print_updated() and show_merge_in_progress() set
it (which is what currently happens). To set the committable bit
correctly, however, wt_status_collect() needs to know whether or not
there is a merge in progress (if a merge is in progress, the bit (1)
should not be set if there are unresolved merge conflicts and (2) should
be set even if the index is the same as HEAD), so teach its (two)
callers to create, initialize, and pass in
instances of wt_status_state, which records this metadata.

Since wt_longstatus_print() and show_merge_in_progress() are in the same
callpaths and currently create and init copies of wt_status_state,
remove that logic and instead pass wt_status_state through.

Make wt_status_get_state() easier to use, add a helper method to clean up
wt_status_state, const-ify as many struct pointers in method signatures
as possible, and add a FIXME for a struct pointer which should be const
but isn't (that this patch series will not address).

Signed-off-by: Samuel Lijin 
---
gitster: I kept the FIXME around because it wasn't clear whether or not
you were opposed to it. For what it's worth, there are only two
callsites that can't be const-ified because of this one item.

 builtin/commit.c |  14 ++--
 ref-filter.c |   3 +-
 wt-status.c  | 178 +++
 wt-status.h  |  11 +--
 4 files changed, 105 insertions(+), 101 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 32f9db33b..dd3e83053 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -485,6 +485,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
 static int run_status(FILE *fp, const char *index_file, const char *prefix, 
int nowarn,
  struct wt_status *s)
 {
+   struct wt_status_state state;
struct object_id oid;
 
if (s->relative_paths)
@@ -504,8 +505,10 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->status_format = status_format;
s->ignore_submodule_arg = ignore_submodule_arg;
 
-   wt_status_collect(s);
-   wt_status_print(s);
+   wt_status_get_state(s, );
+   wt_status_collect(s, );
+   wt_status_print(s, );
+   wt_status_clear_state();
 
return s->committable;
 }
@@ -1295,6 +1298,7 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
static int no_renames = -1;
static const char *rename_score_arg = (const char *)-1;
static struct wt_status s;
+   struct wt_status_state state;
int fd;
struct object_id oid;
static struct option builtin_status_options[] = {
@@ -1379,7 +1383,8 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
s.rename_score = parse_rename_score(_score_arg);
}
 
-   wt_status_collect();
+   wt_status_get_state(, );
+   wt_status_collect(, );
 
if (0 <= fd)
update_index_if_able(_index, _lock);
@@ -1387,7 +1392,8 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
if (s.relative_paths)
s.prefix = prefix;
 
-   wt_status_print();
+   wt_status_print(, );
+   wt_status_clear_state();
return 0;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index 492f2b770..bc9b6b274 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1381,8 +1381,7 @@ char *get_head_description(void)
 {
struct strbuf desc = STRBUF_INIT;
struct wt_status_state state;
-   memset(, 0, sizeof(state));
-   wt_status_get_state(, 1);
+   wt_status_get_state(NULL, );
if (state.rebase_in_progress ||
state.rebase_interactive_in_progress) {
if (state.branch)
diff --git a/wt-status.c b/wt-status.c
index 18ea333a5..af83fae68 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -33,7 +33,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
GIT_COLOR_NIL,/* WT_STATUS_ONBRANCH */
 };
 
-static const char *color(int slot, struct wt_status *s)
+static const char *color(int slot, const struct wt_status *s)
 {
const char *c = "";
if (want_color(s->use_color))
@@ -43,7 +43,7 @@ static const char *color(int slot, struct wt_status *s)
return c;
 }
 
-static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+static void status_vprintf(const struct wt_status *s, int at_bol, const char 
*color,
const char *fmt, va_list ap, const char *trail)
 {
struct strbuf sb = STRBUF_INIT;
@@ -89,7 +89,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, 
const char *color,
strbuf_release();
 }
 
-void status_printf_ln(struct wt_status *s, const char *color,
+void status_printf_ln(const struct wt_

Re: [PATCH v3 3/3] commit: fix exit code for --short/--porcelain

[Samuel Lijin]

Thanks for the review.

On Tue, Jul 17, 2018 at 10:33 AM Junio C Hamano  wrote:
>
> Samuel Lijin  writes:
>
> > diff --git a/wt-status.c b/wt-status.c
> > index 75d389944..4ba657978 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -718,6 +718,39 @@ static void wt_status_collect_untracked(struct 
> > wt_status *s)
> >   s->untracked_in_ms = (getnanotime() - t_begin) / 100;
> >  }
> >
> > +static int has_unmerged(const struct wt_status *s)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < s->change.nr; i++) {
> > + struct wt_status_change_data *d;
> > + d = s->change.items[i].util;
> > + if (d->stagemask)
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > +static void wt_status_mark_committable(
> > + struct wt_status *s, const struct wt_status_state *state)
> > +{
> > + int i;
> > +
> > + if (state->merge_in_progress && !has_unmerged(s)) {
> > + s->committable = 1;
> > + return;
> > + }
>
> Is this trying to say:
>
> During/after a merge, if there is no higher stage entry in
> the index, we can commit.
>
> I am wondering if we also should say:
>
> During/after a merge, if there is any unresolved conflict in
> the index, we cannot commit.
>
> in which case the above becomes more like this:
>
> if (state->merge_in_progress) {
> s->committable = !has_unmerged(s);
> return;
> }
>
> But with your patch, with no remaining conflict in the index during
> a merge, the control comes here and goes into the next loop.
>
> > + for (i = 0; i < s->change.nr; i++) {
> > + struct wt_status_change_data *d = (s->change.items[i]).util;
> > +
> > + if (d->index_status && d->index_status != 
> > DIFF_STATUS_UNMERGED) {
> > + s->committable = 1;
> > + return;
> > + }
> > + }
>
> The loop seems to say "As long as there is one entry in the index
> that is not in conflict and is different from the HEAD, then we can
> commit".  Is that correct?
>
> Imagine there are two paths A and B in the branches involved in a
> merge, and A cleanly resolves (say, we take their version because
> our history did not touch it since we diverged) while B has
> conflict.  We'll come to this loop (because we are in a merge but
> have some unmerged paths) and we find that A is different from HEAD,
> happily set committable bit and return.

I'll be honest: when I wrote this, I didn't think too much about what
the code was actually doing, semantically speaking: I was assuming
that the behavior that set the commitable bit in the call tree of
wt_longstatus_print() was correct, and that it was just a matter of
mechanically copying that logic over to the --short/--porcelain call
paths.

Looking into this more deeply, I think you're right, but more
problematically, this is technically a bug with the current Git code
that seems to be cancelled out by another bug: wt_status_state
apparently does not correctly reflect the state of the index when it
reaches wt_longstatus_print_updated(). Working from master
(f55ff46c9), I modified the last test in t7501 to look like this:

→.echo "Initial contents, unimportant" | tee test-file1 test-file2 &&
→.git add test-file1 test-file2 &&
→.git commit -m "Initial commit" &&
→.echo "commit-1-state" | tee test-file1 test-file2 &&
→.git commit -m "commit 1" -i test-file1 test-file2 &&
→.git tag commit-1 &&
→.git checkout -b branch-2 HEAD^1 &&
→.echo "commit-2-state" | tee test-file1 test-file2 &&
→.git commit -m "commit 2" -i test-file1 test-file2 &&
→.! $(git merge --no-commit commit-1) &&
→.echo "commit-2-state" | tee test-file1 &&
→.git add test-file1 &&
→.git commit --dry-run &&
→.git commit -m "conflicts fixed from merge."

And once inside gdb did this:

(gdb) b wt-status.c:766
Breakpoint 1 at 0x205d73: file wt-status.c, line 766.
(gdb) r
Starting program: /home/pockets/git/git commit --dry-run
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
On branch branch-2
You have unmerged paths.
  (fix conflicts and run "git commit")
  (use "git merge --abort" to abort the merge)


Breakpoint 1, wt_longstatus_print_updated (s=0x555

[PATCH v3 1/3] t7501: add merge conflict tests for dry run

[Samuel Lijin]

The behavior of git commit when doing a dry run changes if there are
unfixed/fixed merge conflits, but the test suite currently only asserts
that `git commit --dry-run` succeeds when all merge conflicts are fixed.

Add tests to document the behavior of all flags which imply a dry run
when (1) there is at least one unfixed merge conflict and (2) when all
merge conflicts are all fixed.

Signed-off-by: Samuel Lijin 
---
 t/t7501-commit.sh | 45 -
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index fa61b1a4e..be087e73f 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -652,7 +652,8 @@ test_expect_success '--only works on to-be-born branch' '
test_cmp expected actual
 '
 
-test_expect_success '--dry-run with conflicts fixed from a merge' '
+# set up env for tests of --dry-run given fixed/unfixed merge conflicts
+test_expect_success 'setup env with unfixed merge conflicts' '
# setup two branches with conflicting information
# in the same file, resolve the conflict,
# call commit with --dry-run
@@ -665,11 +666,45 @@ test_expect_success '--dry-run with conflicts fixed from 
a merge' '
git checkout -b branch-2 HEAD^1 &&
echo "commit-2-state" >test-file &&
git commit -m "commit 2" -i test-file &&
-   ! $(git merge --no-commit commit-1) &&
-   echo "commit-2-state" >test-file &&
+   test_expect_code 1 git merge --no-commit commit-1
+'
+
+test_expect_success '--dry-run with unfixed merge conflicts' '
+   test_expect_code 1 git commit --dry-run
+'
+
+test_expect_success '--short with unfixed merge conflicts' '
+   test_expect_code 1 git commit --short
+'
+
+test_expect_success '--porcelain with unfixed merge conflicts' '
+   test_expect_code 1 git commit --porcelain
+'
+
+test_expect_success '--long with unfixed merge conflicts' '
+   test_expect_code 1 git commit --long
+'
+
+test_expect_success '--dry-run with conflicts fixed from a merge' '
+   echo "merge-conflicts-fixed" >test-file &&
git add test-file &&
-   git commit --dry-run &&
-   git commit -m "conflicts fixed from merge."
+   git commit --dry-run
+'
+
+test_expect_failure '--short with conflicts fixed from a merge' '
+   git commit --short
+'
+
+test_expect_failure '--porcelain with conflicts fixed from a merge' '
+   git commit --porcelain
+'
+
+test_expect_success '--long with conflicts fixed from a merge' '
+   git commit --long
+'
+
+test_expect_success '--message with conflicts fixed from a merge' '
+   git commit --message "conflicts fixed from merge."
 '
 
 test_done
-- 
2.18.0

[PATCH v3 2/3] wt-status: teach wt_status_collect about merges in progress

[Samuel Lijin]

To fix the breakages documented by t7501, the next patch in this series
will teach wt_status_collect() to set the committable bit, instead of
having wt_longstatus_print_updated() and show_merge_in_progress() set it
(which is what currently happens). Unfortunately, wt_status_collect()
needs to know whether or not there is a merge in progress to set the bit
correctly, so teach its (two) callers to create, initialize, and pass
in instances of wt_status_state, which records this metadata.

Since wt_longstatus_print() and show_merge_in_progress() are in the same
callpaths and currently create and init copies of wt_status_state,
remove that logic and instead pass wt_status_state through.

Make wt_status_get_state easier to use, add a helper method to clean up
wt_status_state, const-ify as many struct pointers in method signatures
as possible, and add a FIXME for a struct pointer which should be const
but isn't (that this patch series will not address).

Signed-off-by: Samuel Lijin 
---
 builtin/commit.c |  32 
 ref-filter.c |   3 +-
 wt-status.c  | 188 +++
 wt-status.h  |  13 ++--
 4 files changed, 120 insertions(+), 116 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 37fcb55ab..79ef4f11a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -463,6 +463,7 @@ static const char *prepare_index(int argc, const char 
**argv, const char *prefix
 static int run_status(FILE *fp, const char *index_file, const char *prefix, 
int nowarn,
  struct wt_status *s)
 {
+   struct wt_status_state state;
struct object_id oid;
 
if (s->relative_paths)
@@ -482,10 +483,12 @@ static int run_status(FILE *fp, const char *index_file, 
const char *prefix, int
s->status_format = status_format;
s->ignore_submodule_arg = ignore_submodule_arg;
 
-   wt_status_collect(s);
-   wt_status_print(s);
+   wt_status_get_state(s, );
+   wt_status_collect(s, );
+   wt_status_print(s, );
+   wt_status_clear_state();
 
-   return s->commitable;
+   return s->committable;
 }
 
 static int is_a_merge(const struct commit *current_head)
@@ -631,7 +634,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
 {
struct stat statbuf;
struct strbuf committer_ident = STRBUF_INIT;
-   int commitable;
+   int committable;
struct strbuf sb = STRBUF_INIT;
const char *hook_arg1 = NULL;
const char *hook_arg2 = NULL;
@@ -848,7 +851,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
 
saved_color_setting = s->use_color;
s->use_color = 0;
-   commitable = run_status(s->fp, index_file, prefix, 1, s);
+   committable = run_status(s->fp, index_file, prefix, 1, s);
s->use_color = saved_color_setting;
} else {
struct object_id oid;
@@ -866,7 +869,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
for (i = 0; i < active_nr; i++)
if (ce_intent_to_add(active_cache[i]))
ita_nr++;
-   commitable = active_nr - ita_nr > 0;
+   committable = active_nr - ita_nr > 0;
} else {
/*
 * Unless the user did explicitly request a submodule
@@ -882,7 +885,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
if (ignore_submodule_arg &&
!strcmp(ignore_submodule_arg, "all"))
flags.ignore_submodules = 1;
-   commitable = index_differs_from(parent, , 1);
+   committable = index_differs_from(parent, , 1);
}
}
strbuf_release(_ident);
@@ -894,7 +897,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
 * explicit --allow-empty. In the cherry-pick case, it may be
 * empty due to conflict resolution, which the user should okay.
 */
-   if (!commitable && whence != FROM_MERGE && !allow_empty &&
+   if (!committable && whence != FROM_MERGE && !allow_empty &&
!(amend && is_a_merge(current_head))) {
s->display_comment_prefix = old_display_comment_prefix;
run_status(stdout, index_file, prefix, 0, s);
@@ -1164,14 +1167,14 @@ static int parse_and_validate_options(int argc, const 
char *argv[],
 static int dry_run_commit(int argc, const char **argv, const char *prefix,
  const struct commit *current_head, struct wt_status 
*s)
 {
-   int commitable;
+   int committable;
const char *index_fil

[PATCH v3 3/3] commit: fix exit code for --short/--porcelain

[Samuel Lijin]

In wt-status.c, the s->commitable bit is set only in the call tree of
wt_longstatus_print(), which means that when there are changes to be
committed or all merge conflicts have been resolved, --dry-run and
--long return the correct exit code, but --short and --porcelain do not,
even though they both imply --dry-run.

Teach wt_status_collect() to set s->committable correctly so that
--short and --porcelain return the correct exit code in the above
described situations and mark the documenting tests as fixed.

Also stop setting s->committable in wt_longstatus_print_updated() and
show_merge_in_progress(), and const-ify wt_status_state in the method
signatures in those callpaths.

Signed-off-by: Samuel Lijin 
---
 t/t7501-commit.sh |  8 ++---
 wt-status.c   | 82 +--
 2 files changed, 55 insertions(+), 35 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index be087e73f..b6492322f 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -87,12 +87,12 @@ test_expect_success '--dry-run with stuff to commit returns 
ok' '
git commit -m next -a --dry-run
 '
 
-test_expect_failure '--short with stuff to commit returns ok' '
+test_expect_success '--short with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --short
 '
 
-test_expect_failure '--porcelain with stuff to commit returns ok' '
+test_expect_success '--porcelain with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --porcelain
 '
@@ -691,11 +691,11 @@ test_expect_success '--dry-run with conflicts fixed from 
a merge' '
git commit --dry-run
 '
 
-test_expect_failure '--short with conflicts fixed from a merge' '
+test_expect_success '--short with conflicts fixed from a merge' '
git commit --short
 '
 
-test_expect_failure '--porcelain with conflicts fixed from a merge' '
+test_expect_success '--porcelain with conflicts fixed from a merge' '
git commit --porcelain
 '
 
diff --git a/wt-status.c b/wt-status.c
index 75d389944..4ba657978 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -718,6 +718,39 @@ static void wt_status_collect_untracked(struct wt_status 
*s)
s->untracked_in_ms = (getnanotime() - t_begin) / 100;
 }
 
+static int has_unmerged(const struct wt_status *s)
+{
+   int i;
+
+   for (i = 0; i < s->change.nr; i++) {
+   struct wt_status_change_data *d;
+   d = s->change.items[i].util;
+   if (d->stagemask)
+   return 1;
+   }
+   return 0;
+}
+
+static void wt_status_mark_committable(
+   struct wt_status *s, const struct wt_status_state *state)
+{
+   int i;
+
+   if (state->merge_in_progress && !has_unmerged(s)) {
+   s->committable = 1;
+   return;
+   }
+
+   for (i = 0; i < s->change.nr; i++) {
+   struct wt_status_change_data *d = (s->change.items[i]).util;
+
+   if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) 
{
+   s->committable = 1;
+   return;
+   }
+   }
+}
+
 void wt_status_collect(struct wt_status *s, const struct wt_status_state 
*state)
 {
wt_status_collect_changes_worktree(s);
@@ -728,6 +761,8 @@ void wt_status_collect(struct wt_status *s, const struct 
wt_status_state *state)
wt_status_collect_changes_index(s);
 
wt_status_collect_untracked(s);
+
+   wt_status_mark_committable(s, state);
 }
 
 static void wt_longstatus_print_unmerged(const struct wt_status *s)
@@ -753,28 +788,28 @@ static void wt_longstatus_print_unmerged(const struct 
wt_status *s)
 
 }
 
-static void wt_longstatus_print_updated(struct wt_status *s)
+static void wt_longstatus_print_updated(const struct wt_status *s)
 {
-   int shown_header = 0;
int i;
 
+   if (!s->committable) {
+   return;
+   }
+
+   wt_longstatus_print_cached_header(s);
+
for (i = 0; i < s->change.nr; i++) {
struct wt_status_change_data *d;
struct string_list_item *it;
it = &(s->change.items[i]);
d = it->util;
-   if (!d->index_status ||
-   d->index_status == DIFF_STATUS_UNMERGED)
-   continue;
-   if (!shown_header) {
-   wt_longstatus_print_cached_header(s);
-   s->committable = 1;
-   shown_header = 1;
+   if (d->index_status &&
+   d->index_status != DIFF_STATUS_UNMERGED) {
+   wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, 
it);
}
-   wt_longstatus_print_change_data(s, WT_STATUS_UPD

[PATCH v3 0/3] Fix --short/--porcelain options for git commit

[Samuel Lijin]

Take 3. Addressed the issue that Junio turned up the last time I sent
this out for review.

I'm not entirely sure I like the way I added the tests in the first
patch, but it's unclear to me if there's actually a pattern for setting
up and tearing down the same env for multiple test methods. There are
also other tests in t7501 that rely on state left from earlier tests, so
it's not really clear to me what the best thing to do here is.

Also added a FIXME in the second patch for something I think should be
fixed, but doesn't make sense to fix in this patch series.

Samuel Lijin (3):
  t7501: add merge conflict tests for dry run
  wt-status: teach wt_status_collect about merges in progress
  commit: fix exit code for --short/--porcelain

 builtin/commit.c  |  32 +++---
 ref-filter.c  |   3 +-
 t/t7501-commit.sh |  49 +++--
 wt-status.c   | 260 +-
 wt-status.h   |  13 +--
 5 files changed, 208 insertions(+), 149 deletions(-)

-- 
2.18.0

Re: [PATCH v2 1/2] commit: fix --short and --porcelain options

[Samuel Lijin]

On Tue, May 1, 2018 at 10:50 PM Junio C Hamano <gits...@pobox.com> wrote:

> Samuel Lijin <sxli...@gmail.com> writes:

> > Mark the commitable flag in the wt_status object in the call to
> > `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
> > and simplify the logic in the latter function to take advantage of the
> > logic shifted to the former. This means that callers do not need to use
> > `wt_longstatus_print_updated()` to collect the `commitable` flag;
> > calling `wt_status_collect()` is sufficient.
> >
> > As a result, invoking `git commit` with `--short` or `--porcelain`
> > (which imply `--dry-run`, but previously returned an inconsistent error
> > code inconsistent with dry run behavior) correctly returns status code
> > zero when there is something to commit. This fixes two bugs documented
> > in the test suite.


> Hmm, I couldn't quite get what the above two paragraphs were trying
> to say, but I think I figured out by looking at wt_status.c before
> applying this patch, so let me see if I correctly understand what
> this patch is about by thinking a bit aloud.

> There are only two assignments to s->commitable in wt-status.c; one
> happens in wt_longstatus_print_updated(), when the function notices
> there is even one record to be shown (i.e. there is an "updated"
> path) and the other in show_merge_in_progress() which is called by
> wt_longstatus_prpint_state().  The latter codepath happens when we
> are in a merge and there is no remaining conflicted paths (the code
> allows the contents to be committed to be identical to HEAD).  Both
> are called from wt_longstatus_print(), which in turn is called by
> wt_status_print().

> The implication of the above observation is that we do not set
> commitable bit (by the way, shouldn't we spell it with two 'T's?)

Yep, MW confirms: https://www.merriam-webster.com/dictionary/commitable

I didn't think to check how common "commitable" is in the codebase, but it
doesn't seem to be too many, looking at the output of `git grep
commitable`, so I'll add that to the patch series when I reroll.

> if we are not doing the long format status.  The title hints at it
> but "fix" is too vague.  It would be easier to understand if it
> began like this (i.e. state problem clearly first, before outlining
> the solution):

>  [PATCH 1/2] commit: fix exit status under --short/--porcelain
options

>  In wt-status.c, s->commitable bit is set only in the
>  codepaths reachable from wt_status_print() when output
>  format is STATUS_FORMAT_LONG as a side effect of printing
>  bits of status.  Consequently, when running with --short and
>  --porcelain options, the bit is not set to reflect if there
>  is anything to be committed, and "git commit --short" or
>  "--porcelain" (both of which imply "--dry-run") failed to
>  signal if there is anything to commit with its exit status.

>  Instead, update s->commitable bit in wt_status_collect(),
>  regardless of the output format. ...

> Is that what is going on here?  Yours made it sound as if moving the
> code to _collect() was done for the sake of moving code around and
> simplifying the logic, and bugfix fell out of the move merely as a
> side effect, which probably was the source of my confusion.

Yep, that's right. I wasn't sure if the imperative tone was required for
the whole commit or just the description, hence the awkward structure. (I
also wasn't sure how strict the 70 char limit on the description was.)

> > +static void wt_status_mark_commitable(struct wt_status *s) {
> > + int i;
> > +
> > + for (i = 0; i < s->change.nr; i++) {
> > + struct wt_status_change_data *d =
(s->change.items[i]).util;
> > +
> > + if (d->index_status && d->index_status !=
DIFF_STATUS_UNMERGED) {
> > + s->commitable = 1;
> > + return;
> > + }
> > + }
> > +}

> I am not sure if this is sufficient.  From a cursory look of the
> existing code (and vague recollection in my ageing brain ;-), I
> think we say it is committable if

>   (1) when not merging, there is something to show in the "to be
>   committed" section (i.e. there must be something changed since
>   HEAD in the index).

>   (2) when merging, no conflicting paths remain (i.e. change.nr being
>   zero is fine).

> So it is unclear to me how you are dealing with (2) under "--short"
> option, which does not call show_merge_in_progress() to catch that
> case.

And the answer t

public-inbox.org down?

[Samuel Lijin]

DNS lookup is working but the website (possibly the server) seems to be
down. The TLS handshake is never acked:

$ curl -v https://public-inbox.org
* Rebuilt URL to: https://public-inbox.org/
*   Trying 64.71.152.64...
* TCP_NODELAY set
* Connected to public-inbox.org (64.71.152.64) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
   CAfile: /etc/ssl/certs/ca-certificates.crt
   CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):

[PATCH v2 2/2] wt-status: const-ify all printf helper methods

[Samuel Lijin]

Change the method signatures of all printf helper methods to take a
`const struct wt_status *` rather than a `struct wt_status *`.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 wt-status.c | 18 +-
 wt-status.h |  4 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 2e5452731..4360bbfd7 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -33,7 +33,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
GIT_COLOR_NIL,/* WT_STATUS_ONBRANCH */
 };
 
-static const char *color(int slot, struct wt_status *s)
+static const char *color(int slot, const struct wt_status *s)
 {
const char *c = "";
if (want_color(s->use_color))
@@ -43,7 +43,7 @@ static const char *color(int slot, struct wt_status *s)
return c;
 }
 
-static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+static void status_vprintf(const struct wt_status *s, int at_bol, const char 
*color,
const char *fmt, va_list ap, const char *trail)
 {
struct strbuf sb = STRBUF_INIT;
@@ -89,7 +89,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, 
const char *color,
strbuf_release();
 }
 
-void status_printf_ln(struct wt_status *s, const char *color,
+void status_printf_ln(const struct wt_status *s, const char *color,
const char *fmt, ...)
 {
va_list ap;
@@ -99,7 +99,7 @@ void status_printf_ln(struct wt_status *s, const char *color,
va_end(ap);
 }
 
-void status_printf(struct wt_status *s, const char *color,
+void status_printf(const struct wt_status *s, const char *color,
const char *fmt, ...)
 {
va_list ap;
@@ -109,7 +109,7 @@ void status_printf(struct wt_status *s, const char *color,
va_end(ap);
 }
 
-static void status_printf_more(struct wt_status *s, const char *color,
+static void status_printf_more(const struct wt_status *s, const char *color,
   const char *fmt, ...)
 {
va_list ap;
@@ -192,7 +192,7 @@ static void wt_longstatus_print_unmerged_header(struct 
wt_status *s)
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(const struct wt_status *s)
 {
const char *c = color(WT_STATUS_HEADER, s);
 
@@ -239,7 +239,7 @@ static void wt_longstatus_print_other_header(struct 
wt_status *s,
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(const struct wt_status *s)
 {
status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -332,7 +332,7 @@ static void wt_longstatus_print_unmerged_data(struct 
wt_status *s,
strbuf_release();
 }
 
-static void wt_longstatus_print_change_data(struct wt_status *s,
+static void wt_longstatus_print_change_data(const struct wt_status *s,
int change_type,
struct string_list_item *it)
 {
@@ -768,7 +768,7 @@ static void wt_longstatus_print_unmerged(struct wt_status 
*s)
 
 }
 
-static void wt_longstatus_print_updated(struct wt_status *s)
+static void wt_longstatus_print_updated(const struct wt_status *s)
 {
int i;
 
diff --git a/wt-status.h b/wt-status.h
index 430770b85..83a1f7c00 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -135,9 +135,9 @@ int wt_status_check_bisect(const struct worktree *wt,
   struct wt_status_state *state);
 
 __attribute__((format (printf, 3, 4)))
-void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, 
...);
+void status_printf_ln(const struct wt_status *s, const char *color, const char 
*fmt, ...);
 __attribute__((format (printf, 3, 4)))
-void status_printf(struct wt_status *s, const char *color, const char *fmt, 
...);
+void status_printf(const struct wt_status *s, const char *color, const char 
*fmt, ...);
 
 /* The following functions expect that the caller took care of reading the 
index. */
 int has_unstaged_changes(int ignore_submodules);
-- 
2.17.0

[PATCH v2 1/2] commit: fix --short and --porcelain options

[Samuel Lijin]

Mark the commitable flag in the wt_status object in the call to
`wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
and simplify the logic in the latter function to take advantage of the
logic shifted to the former. This means that callers do not need to use
`wt_longstatus_print_updated()` to collect the `commitable` flag;
calling `wt_status_collect()` is sufficient.

As a result, invoking `git commit` with `--short` or `--porcelain`
(which imply `--dry-run`, but previously returned an inconsistent error
code inconsistent with dry run behavior) correctly returns status code
zero when there is something to commit. This fixes two bugs documented
in the test suite.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7501-commit.sh |  4 ++--
 wt-status.c   | 38 +++---
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index fa61b1a4e..85a8217fd 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -87,12 +87,12 @@ test_expect_success '--dry-run with stuff to commit returns 
ok' '
git commit -m next -a --dry-run
 '
 
-test_expect_failure '--short with stuff to commit returns ok' '
+test_expect_success '--short with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --short
 '
 
-test_expect_failure '--porcelain with stuff to commit returns ok' '
+test_expect_success '--porcelain with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --porcelain
 '
diff --git a/wt-status.c b/wt-status.c
index 50815e5fa..2e5452731 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -718,6 +718,19 @@ static void wt_status_collect_untracked(struct wt_status 
*s)
s->untracked_in_ms = (getnanotime() - t_begin) / 100;
 }
 
+static void wt_status_mark_commitable(struct wt_status *s) {
+   int i;
+
+   for (i = 0; i < s->change.nr; i++) {
+   struct wt_status_change_data *d = (s->change.items[i]).util;
+
+   if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) 
{
+   s->commitable = 1;
+   return;
+   }
+   }
+}
+
 void wt_status_collect(struct wt_status *s)
 {
wt_status_collect_changes_worktree(s);
@@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
wt_status_collect_changes_initial(s);
else
wt_status_collect_changes_index(s);
+
wt_status_collect_untracked(s);
+
+   wt_status_mark_commitable(s);
 }
 
 static void wt_longstatus_print_unmerged(struct wt_status *s)
@@ -754,26 +770,26 @@ static void wt_longstatus_print_unmerged(struct wt_status 
*s)
 
 static void wt_longstatus_print_updated(struct wt_status *s)
 {
-   int shown_header = 0;
int i;
 
+   if (!s->commitable) {
+   return;
+   }
+
+   wt_longstatus_print_cached_header(s);
+
for (i = 0; i < s->change.nr; i++) {
struct wt_status_change_data *d;
struct string_list_item *it;
it = &(s->change.items[i]);
d = it->util;
-   if (!d->index_status ||
-   d->index_status == DIFF_STATUS_UNMERGED)
-   continue;
-   if (!shown_header) {
-   wt_longstatus_print_cached_header(s);
-   s->commitable = 1;
-   shown_header = 1;
+   if (d->index_status &&
+   d->index_status != DIFF_STATUS_UNMERGED) {
+   wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, 
it);
}
-   wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
}
-   if (shown_header)
-   wt_longstatus_print_trailer(s);
+
+   wt_longstatus_print_trailer(s);
 }
 
 /*
-- 
2.17.0

[PATCH v2 0/2] Fix --short and --porcelain options for commit

[Samuel Lijin]

Rerolling patch series to fix t7501.

Samuel Lijin (2):
  commit: fix --short and --porcelain options
  wt-status: const-ify all printf helper methods

 t/t7501-commit.sh |  4 ++--
 wt-status.c   | 56 ++-
 wt-status.h   |  4 ++--
 3 files changed, 40 insertions(+), 24 deletions(-)

-- 
2.17.0

Re: [PATCH 1/2] commit: fix --short and --porcelain

[Samuel Lijin]

On Wed, Apr 18, 2018 at 8:55 PM, Samuel Lijin <sxli...@gmail.com> wrote:
> Thanks for the quick review!
>
> On Wed, Apr 18, 2018 at 11:38 AM, Martin Ågren <martin.ag...@gmail.com> wrote:
>> Hi Samuel,
>>
>> Welcome back. :-)
>>
>> On 18 April 2018 at 05:06, Samuel Lijin <sxli...@gmail.com> wrote:
>>> Make invoking `git commit` with `--short` or `--porcelain` return status
>>> code zero when there is something to commit.
>>>
>>> Mark the commitable flag in the wt_status object in the call to
>>> `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
>>> and simplify the logic in the latter function to take advantage of the
>>> logic shifted to the former.
>>
>> The subject is sort of vague about what is being fixed. Maybe "commit:
>> fix return code of ...", or "wt-status: set `commitable` when
>> collecting, not when printing". Or something... I can't come up with
>> something brilliant off the top of my head.
>>
>> I did not understand the first paragraph until I had read the second and
>> peaked at the code. Maybe tell the story the other way around? Something
>> like this:
>>
>>   Mark the `commitable` flag in the wt_status object in
>>   `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
>>   and simplify the logic in the latter function to take advantage of the
>>   logic shifted to the former.
>>
>>   This means that callers do need to actually use the printer function
>>   to collect the `commitable` flag -- it is sufficient to call
>>   `wt_status_collect()`.
>>
>>   As a result, invoking `git commit` with `--short` or `--porcelain`
>>   results in return status code zero when there is something to commit.
>>   This fixes two bugs documented in our test suite.
>
> That definitely works better. Will fix when I reroll.
>
>>>  t/t7501-commit.sh |  4 ++--
>>>  wt-status.c   | 39 +++
>>>  2 files changed, 29 insertions(+), 14 deletions(-)
>>
>> I tried to find somewhere in the documentation where this bug was
>> described (git-commit.txt or git-status.txt), but failed. So there
>> should be nothing to update there.
>>
>>> +static void wt_status_mark_commitable(struct wt_status *s) {
>>> +   int i;
>>> +
>>> +   for (i = 0; i < s->change.nr; i++) {
>>> +   struct wt_status_change_data *d = (s->change.items[i]).util;
>>> +
>>> +   if (d->index_status && d->index_status != 
>>> DIFF_STATUS_UNMERGED) {
>>> +   s->commitable = 1;
>>> +   return;
>>> +   }
>>> +   }
>>> +}
>>
>> This helper does exactly what the old code did inside
>> `wt_longstatus_print_updated()` with regards to `commitable`. Ok.
>>
>> This function does not reset `commitable` to 0, so reusing a `struct
>> wt_status` won't necessarily work out. I have not thought about whether
>> such a caller would be horribly broken for other reasons...
>>
>>>  void wt_status_collect(struct wt_status *s)
>>>  {
>>> wt_status_collect_changes_worktree(s);
>>> @@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
>>> wt_status_collect_changes_initial(s);
>>> else
>>> wt_status_collect_changes_index(s);
>>> +
>>> wt_status_collect_untracked(s);
>>> +
>>> +   wt_status_mark_commitable(s);
>>>  }
>>
>> So whenever we `..._collect()`, `commitable` is set for us. This is the
>> only caller of the new helper, so in order to be able to trust
>> `commitable`, one needs to call `wt_status_collect()`. Seems a
>> reasonable assumption to make that the caller will remember to do so
>> before printing. (And all current users do, so we're not regressing in
>> some user.)
>>
>>>  static void wt_longstatus_print_unmerged(struct wt_status *s)
>>> @@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct 
>>> wt_status *s)
>>>
>>>  static void wt_longstatus_print_updated(struct wt_status *s)
>>>  {
>>> -   int shown_header = 0;
>>> -   int i;
>>> +   if (!s->commitable) {
>>> +   return;
>>> +   }
>>
>> Regarding my comment above: If you forget to `..._collect()` first, this
>> function is a no-op.
>&

[PATCH 0/2] Fix --short and --porcelain options for commit

[Samuel Lijin]

Hi all - I last contributed about a year ago and I've finally found the
time to start contributing again, and hopefully I'll stick around this
time. Figured I'd start with something small :)

Samuel Lijin (2):
  commit: fix --short and --porcelain
  wt-status: const-ify all printf helper methods

 t/t7501-commit.sh |  4 ++--
 wt-status.c   | 57 +++
 wt-status.h   |  4 ++--
 3 files changed, 40 insertions(+), 25 deletions(-)

-- 
2.16.2

[PATCH 2/2] wt-status: const-ify all printf helper methods

[Samuel Lijin]

Change the method signatures of all printf helper methods to take a
`const struct wt_status *` rather than a `struct wt_status *`.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 wt-status.c | 18 +-
 wt-status.h |  4 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 26b0a6221..55d29bc09 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -33,7 +33,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
GIT_COLOR_NIL,/* WT_STATUS_ONBRANCH */
 };
 
-static const char *color(int slot, struct wt_status *s)
+static const char *color(int slot, const struct wt_status *s)
 {
const char *c = "";
if (want_color(s->use_color))
@@ -43,7 +43,7 @@ static const char *color(int slot, struct wt_status *s)
return c;
 }
 
-static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+static void status_vprintf(const struct wt_status *s, int at_bol, const char 
*color,
const char *fmt, va_list ap, const char *trail)
 {
struct strbuf sb = STRBUF_INIT;
@@ -89,7 +89,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, 
const char *color,
strbuf_release();
 }
 
-void status_printf_ln(struct wt_status *s, const char *color,
+void status_printf_ln(const struct wt_status *s, const char *color,
const char *fmt, ...)
 {
va_list ap;
@@ -99,7 +99,7 @@ void status_printf_ln(struct wt_status *s, const char *color,
va_end(ap);
 }
 
-void status_printf(struct wt_status *s, const char *color,
+void status_printf(const struct wt_status *s, const char *color,
const char *fmt, ...)
 {
va_list ap;
@@ -109,7 +109,7 @@ void status_printf(struct wt_status *s, const char *color,
va_end(ap);
 }
 
-static void status_printf_more(struct wt_status *s, const char *color,
+static void status_printf_more(const struct wt_status *s, const char *color,
   const char *fmt, ...)
 {
va_list ap;
@@ -192,7 +192,7 @@ static void wt_longstatus_print_unmerged_header(struct 
wt_status *s)
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(const struct wt_status *s)
 {
const char *c = color(WT_STATUS_HEADER, s);
 
@@ -239,7 +239,7 @@ static void wt_longstatus_print_other_header(struct 
wt_status *s,
status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(const struct wt_status *s)
 {
status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -332,7 +332,7 @@ static void wt_longstatus_print_unmerged_data(struct 
wt_status *s,
strbuf_release();
 }
 
-static void wt_longstatus_print_change_data(struct wt_status *s,
+static void wt_longstatus_print_change_data(const struct wt_status *s,
int change_type,
struct string_list_item *it)
 {
@@ -768,7 +768,7 @@ static void wt_longstatus_print_unmerged(struct wt_status 
*s)
 
 }
 
-static void wt_longstatus_print_updated(struct wt_status *s)
+static void wt_longstatus_print_updated(const struct wt_status *s)
 {
if (!s->commitable) {
return;
diff --git a/wt-status.h b/wt-status.h
index 430770b85..83a1f7c00 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -135,9 +135,9 @@ int wt_status_check_bisect(const struct worktree *wt,
   struct wt_status_state *state);
 
 __attribute__((format (printf, 3, 4)))
-void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, 
...);
+void status_printf_ln(const struct wt_status *s, const char *color, const char 
*fmt, ...);
 __attribute__((format (printf, 3, 4)))
-void status_printf(struct wt_status *s, const char *color, const char *fmt, 
...);
+void status_printf(const struct wt_status *s, const char *color, const char 
*fmt, ...);
 
 /* The following functions expect that the caller took care of reading the 
index. */
 int has_unstaged_changes(int ignore_submodules);
-- 
2.16.2

[PATCH 1/2] commit: fix --short and --porcelain

[Samuel Lijin]

Make invoking `git commit` with `--short` or `--porcelain` return status
code zero when there is something to commit.

Mark the commitable flag in the wt_status object in the call to
`wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
and simplify the logic in the latter function to take advantage of the
logic shifted to the former.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7501-commit.sh |  4 ++--
 wt-status.c   | 39 +++
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index fa61b1a4e..85a8217fd 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -87,12 +87,12 @@ test_expect_success '--dry-run with stuff to commit returns 
ok' '
git commit -m next -a --dry-run
 '
 
-test_expect_failure '--short with stuff to commit returns ok' '
+test_expect_success '--short with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --short
 '
 
-test_expect_failure '--porcelain with stuff to commit returns ok' '
+test_expect_success '--porcelain with stuff to commit returns ok' '
echo bongo bongo bongo >>file &&
git commit -m next -a --porcelain
 '
diff --git a/wt-status.c b/wt-status.c
index 50815e5fa..26b0a6221 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -718,6 +718,19 @@ static void wt_status_collect_untracked(struct wt_status 
*s)
s->untracked_in_ms = (getnanotime() - t_begin) / 100;
 }
 
+static void wt_status_mark_commitable(struct wt_status *s) {
+   int i;
+
+   for (i = 0; i < s->change.nr; i++) {
+   struct wt_status_change_data *d = (s->change.items[i]).util;
+
+   if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) 
{
+   s->commitable = 1;
+   return;
+   }
+   }
+}
+
 void wt_status_collect(struct wt_status *s)
 {
wt_status_collect_changes_worktree(s);
@@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
wt_status_collect_changes_initial(s);
else
wt_status_collect_changes_index(s);
+
wt_status_collect_untracked(s);
+
+   wt_status_mark_commitable(s);
 }
 
 static void wt_longstatus_print_unmerged(struct wt_status *s)
@@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct wt_status 
*s)
 
 static void wt_longstatus_print_updated(struct wt_status *s)
 {
-   int shown_header = 0;
-   int i;
+   if (!s->commitable) {
+   return;
+   }
+
+   wt_longstatus_print_cached_header(s);
 
+   int i;
for (i = 0; i < s->change.nr; i++) {
struct wt_status_change_data *d;
struct string_list_item *it;
it = &(s->change.items[i]);
d = it->util;
-   if (!d->index_status ||
-   d->index_status == DIFF_STATUS_UNMERGED)
-   continue;
-   if (!shown_header) {
-   wt_longstatus_print_cached_header(s);
-   s->commitable = 1;
-   shown_header = 1;
+   if (d->index_status &&
+   d->index_status != DIFF_STATUS_UNMERGED) {
+   wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, 
it);
}
-   wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
}
-   if (shown_header)
-   wt_longstatus_print_trailer(s);
+
+   wt_longstatus_print_trailer(s);
 }
 
 /*
-- 
2.16.2

Re: Using '--help' for aliases

[Samuel Lijin]

You could also just make the alias a bash function that does git help
checkout when you pass the --help flag to the alias.

Re: [PATCH v3] mergetools/meld: improve compatibiilty with Meld on macOS X

[Samuel Lijin]

On Sun, Jun 18, 2017 at 10:10 PM, David Aguilar <dav...@gmail.com> wrote:
> The macOS X fork of Meld[1] requires a "=" in the "--output"
> argument, as it uses a wrapper[2] script that munges the
> "--output" argument before calling into the common "meld"
> script.
>
> The macOS X wrapper script[2] accepts "--output="
> only, despite the fact that the underlying meld code accepts
> both "--output <filename" and "--output="[3].
>
> All versions of meld which accept "--output" accept it in
> the "--output=" form, so use "--output=" for
> maximum compatibility.

Aha, then if that's the case this looks fine to me. Thanks!

> [1] https://github.com/yousseb/meld
> [2] https://github.com/yousseb/meld/blob/master/osx/Meld
> [3] https://github.com/yousseb/meld/issues/42
>
> Reported-by: Matthew Groth <mgrot...@gmail.com>
> Helped-by: Samuel Lijin <sxli...@gmail.com>
> Signed-off-by: David Aguilar <dav...@gmail.com>
> ---
> I cloned the meld repo and could not find the code reported in the original
> issue, but I did find that same exact code existed in a macOS fork.
>
> After more investigation, this turned out to be a macOS-only issue.  The
> commit message has been updated to better reflect what's really going on.
>
>  mergetools/meld | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mergetools/meld b/mergetools/meld
> index bc178e8882..7a08470f88 100644
> --- a/mergetools/meld
> +++ b/mergetools/meld
> @@ -10,7 +10,7 @@ merge_cmd () {
>
> if test "$meld_has_output_option" = true
> then
> -   "$merge_tool_path" --output "$MERGED" \
> +   "$merge_tool_path" --output="$MERGED" \
> "$LOCAL" "$BASE" "$REMOTE"
> else
> "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> --
> 2.13.1.453.gc0395165f3
>

Re: [PATCH] mergetools/meld: improve backwards-compatibiilty when using "--output"

[Samuel Lijin]

On Sun, Jun 18, 2017 at 8:17 PM, David Aguilar <dav...@gmail.com> wrote:
> On Sun, Jun 18, 2017 at 05:11:48AM -0400, Samuel Lijin wrote:
>> On Sun, Jun 18, 2017 at 3:46 AM, David Aguilar <dav...@gmail.com> wrote:
>> > On Sat, Jun 17, 2017 at 10:11:36AM -0400, Samuel Lijin wrote:
>> >> On Sat, Jun 17, 2017 at 6:24 AM, David Aguilar <dav...@gmail.com> wrote:
>> >> > Meld 3.16.0 requires a "=" in the --output argument, as it uses
>> >> > a simple hand-rolled command-line parser.
>> >> >
>> >> > Newer versions of Meld (3.16.4, and possibly earlier) use
>> >> > optpaarse, which accepts either "--output " or
>> >> > "--output=".
>> >
>> > Junio, there's an optpaarse -> optparse typo in the commit message
>> > here in case you want to fix that up.
>> >
>> >>
>> >> Do older versions also support both?
>> >
>> > No.  When the "--output" option was first added (3.16.0, or possibly
>> > earlier) it used the simpler parser that does not undertand the
>> > "--output " form.
>> >
>> > Much older versions didn't support "--output" at all, so we don't have
>> > to worry about them since we already use the "--output" flag
>> > selectively based on whether or not it's supported.
>>
>> It sounds like this patch would break versions of Meld that use the
>> hand-rolled parser, then.
>
> I don't think so.
>
> The whole point of this patch is to make it compatible with the
> hand-rolled parser.
>
> Before the patch:
>
> --output 
>
> After the patch:
>
> --output=
>
>
> The form with "=" (the latter one) is the one that's maximally
> compatible.
>
> Please re-read the commit message and patch to verify that this is
> indeed true.

Whoops, sorry, yes, you're right. It does still sound like there are
some versions of Meld in the middle that rely on "--output "
though, that this does break.

Re: [PATCH] mergetools/meld: improve backwards-compatibiilty when using "--output"

[Samuel Lijin]

On Sun, Jun 18, 2017 at 3:46 AM, David Aguilar <dav...@gmail.com> wrote:
> On Sat, Jun 17, 2017 at 10:11:36AM -0400, Samuel Lijin wrote:
>> On Sat, Jun 17, 2017 at 6:24 AM, David Aguilar <dav...@gmail.com> wrote:
>> > Meld 3.16.0 requires a "=" in the --output argument, as it uses
>> > a simple hand-rolled command-line parser.
>> >
>> > Newer versions of Meld (3.16.4, and possibly earlier) use
>> > optpaarse, which accepts either "--output " or
>> > "--output=".
>
> Junio, there's an optpaarse -> optparse typo in the commit message
> here in case you want to fix that up.
>
>>
>> Do older versions also support both?
>
> No.  When the "--output" option was first added (3.16.0, or possibly
> earlier) it used the simpler parser that does not undertand the
> "--output " form.
>
> Much older versions didn't support "--output" at all, so we don't have
> to worry about them since we already use the "--output" flag
> selectively based on whether or not it's supported.

It sounds like this patch would break versions of Meld that use the
hand-rolled parser, then.

Re: [PATCH] mergetools/meld: improve backwards-compatibiilty when using "--output"

[Samuel Lijin]

On Sat, Jun 17, 2017 at 6:24 AM, David Aguilar  wrote:
> Meld 3.16.0 requires a "=" in the --output argument, as it uses
> a simple hand-rolled command-line parser.
>
> Newer versions of Meld (3.16.4, and possibly earlier) use
> optpaarse, which accepts either "--output " or
> "--output=".

Do older versions also support both?

> Use "--output=" for better compatibility.
>
> Signed-off-by: David Aguilar 
> ---
>  mergetools/meld | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mergetools/meld b/mergetools/meld
> index bc178e8882..7a08470f88 100644
> --- a/mergetools/meld
> +++ b/mergetools/meld
> @@ -10,7 +10,7 @@ merge_cmd () {
>
> if test "$meld_has_output_option" = true
> then
> -   "$merge_tool_path" --output "$MERGED" \
> +   "$merge_tool_path" --output="$MERGED" \
> "$LOCAL" "$BASE" "$REMOTE"
> else
> "$merge_tool_path" "$LOCAL" "$MERGED" "$REMOTE"
> --
> 2.13.1.453.gc0395165f3
>

Re: [suggestion] Include commit-ish in git status output

[Samuel Lijin]

On Thu, Jun 15, 2017 at 7:43 PM, Mahmoud Al-Qudsi  wrote:
> Hello all,
>
> I hope it is not considered too forward of me for my first post to this list
> to be a suggestion on a change to git’s behavior (though not in any
> functional manner); but a persistent frustration for me and everyone I’ve
> worked with (so, yes, 100% based off of anecdata) has been that the output
> of `git status` does not include the commit or commit-ish, and one must
> resort to a `git rev-parse HEAD` call.

Can you elaborate on why you consider this useful specifically?

Do you think adding a $(git rev-parse HEAD) to your PS1 would do the trick?

> I apologize unreservedly if this matter has already been discussed and put to
> rest; I attempted to search the archives for a reference to this suggestion 
> but
> was not met with any matches.
>
> Additionally, if this list is not the right place to make such a suggestion,
> then I would appreciate if someone could kindly point me in the correct
> direction and apologize for littering.

No worries, you're in the right place.

> Thank you kindly,
>
> Mahmoud Al-Qudsi
> NeoSmart Technologies

Re: git diff sometimes brings up buggy pager

[Samuel Lijin]

Any chance you can tell us what repo this happens on? + git version,
OS, and what the triggering diff invocation is.

On Thu, Jun 15, 2017 at 12:19 PM, Matthew Groth  wrote:
> When I do `git diff` sometimes I get this:
>
>
> ...skipping...
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ...skipping...
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ...skipping...
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
> ~
>
>
>  it goes on like this for about 10 times the length. Looks like this
> happens exclusively when I use git diff with a github remote that is at the
> same commit. I will update if I find any other case where this happens.
>
>
>
>

Re: [PATCH] wt-status.c: Modified status message shown for a parent-less branch

[Samuel Lijin]

On Thu, Jun 15, 2017 at 4:42 AM, Jeff King  wrote:
>
> On Thu, Jun 15, 2017 at 01:49:20PM +0530, Kaartic Sivaraam wrote:
>
> > What about, "not making any assumptions" about what the user would
> > think when he views the output of `git status` ? Why not try some
> > general messages like,
> >
> > * Staged changes
> > * Unstaged changes
> >
> > instead of the messages such as
> >
> > * Changes to be committed, Changes already in the index
> > * Changes not staged for commit, Changes not yet in the index
> >
> > which seem to make assumptions about the user who view the output ?
>
> Saying just "staged changes" is definitely accurate. I don't know if
> some users would find that too terse, too. The phrase "not staged for
> commit" gives more information if you don't know what "staged" means in
> the Git world.

Perhaps there should be a message pointing people at documentation
explaining the index and staging terminology?

Offhand, this is something I was wondering about the other day - has
there ever been a discussion of what level of proficiency Git expects
of its users?

> -Peff

Re: Feature Request: Show status of the stash in git status command

[Samuel Lijin]

On Sat, Jun 10, 2017 at 4:25 AM, Jeff King  wrote:
> On Wed, Jun 07, 2017 at 06:46:18PM -0400, Houston Fortney wrote:
>
>> I sometimes forget about something that I stashed. It would be nice if
>> the git status command would just say "There are x entries in the
>> stash." It can say nothing if there is nothing stashed so it is
>> usually not adding clutter.
>
> I think the clutter issue would depend on your workflow around stash.
>
> Some people carry tidbits in their stash for days or weeks. E.g., I
> sometimes start on an idea and decide it's not worth pursuing (or more
> likely, I post a snippet of a patch as a "how about this" to the mailing
> list but don't plan on taking it further). Rather than run "git reset
> --hard", I usually "git stash" the result. That means if I really do
> decide I want it back, I can prowl through the stash list and find it.
>
> All of which is to say that if we had such a feature, it should probably
> be optional. For some people it would be very useful, and for others it
> would be a nuisance.

Perhaps there should be a flag for this if it is implemented, say
status.showStash?

> Do you want to try a patch? I think you'd need to find the right spot in
> wt-status.c to show the output, and then call for_each_reflog_ent() on
> "refs/stash" and count the number of entries you see.
>
> -Peff

Clarifying behavior of diff --stat

[Samuel Lijin]

I noticed when answering a StackOverflow question that apparently
--stat modifies the raw diff itself, instead of just adding a diffstat
to the output. In the linux kernel:

$ git diff-tree --raw -M 00a2430ff07d4e0e0e7e24e02fd8adede333b797
00a2430ff07d4e0e0e7e24e02fd8adede333b797
:04 04 f5792c6667c23e113a4f18acfdc5e2c38f5217d7
fb019210ebea7cf439f37d67111797ac7e37cca9 M  drivers

$ git diff-tree --raw --stat -M 00a2430ff07d4e0e0e7e24e02fd8adede333b797
00a2430ff07d4e0e0e7e24e02fd8adede333b797
:100644 100644 c144102ea793541c7e8dad1d42072bb62e71ed4f
a186afeaa7001f3f81abb4f95d9edd3099477867 M
drivers/usb/gadget/Makefile
:00 100644 
6d91f21b52a6301c303aac0a4f62e572d83c02aa A
drivers/usb/gadget/function/Makefile
:100644 100644 ab1065afbbd0c0cac55788dad3b805f9a240b2a9
ab1065afbbd0c0cac55788dad3b805f9a240b2a9 R100
drivers/usb/gadget/f_acm.c  drivers/usb/gadget/function/f_acm.c
:1006
[...]
 drivers/usb/gadget/Makefile| 30 +--
 drivers/usb/gadget/function/Makefile   | 34 ++
 drivers/usb/gadget/{ => function}/f_acm.c  |  0
 [...]

Is this intentional? And is there a way to generate a raw version of
the extended stat without using --stat?

Re: send-email: Net::SSLeay failure

[Samuel Lijin]

On Thu, Jun 8, 2017 at 5:53 AM, Ævar Arnfjörð Bjarmason
<ava...@gmail.com> wrote:
> On Thu, Jun 8, 2017 at 11:06 AM, Samuel Lijin <sxli...@gmail.com> wrote:
>> Sorry, I should've been clearer. Currently send-email fails for me
>> with this error:
>>
>> SSLeay.c: loadable library and perl binaries are mismatched (got
>> handshake key 0xdb80080, needed 0xde00080)
>>
>> I thought the undefined symbol stuff in the .so could have been part
>> of the problem, but I guess not. None of the fixes that worked for
>> Liam worked for me either, so I'm not quite sure where the issue is.
>> The closest I've found to a meaningful suggestion for the error is
>> that Perl is relying on a library built against a different version of
>> Perl, but I have no idea what library that might be.
>
> The issue is that SSLeay.so is compiled against a different version of
> perl, presumably you installed it via CPAN in the past and then
> upgraded your system perl version.
>
> The easiest solution is probably just to re-run "sudo cpan
> Net::SSLeay", or alternatively rm -rf all these files and install them
> via your package system.

I thought I already had rm -rf'd them, but upon double-checking it
turns out I didn't nuke the CPAN version of Net::SSLeay properly.
Thanks.

Re: send-email: Net::SSLeay failure

[Samuel Lijin]

Sorry, I should've been clearer. Currently send-email fails for me
with this error:

SSLeay.c: loadable library and perl binaries are mismatched (got
handshake key 0xdb80080, needed 0xde00080)

I thought the undefined symbol stuff in the .so could have been part
of the problem, but I guess not. None of the fixes that worked for
Liam worked for me either, so I'm not quite sure where the issue is.
The closest I've found to a meaningful suggestion for the error is
that Perl is relying on a library built against a different version of
Perl, but I have no idea what library that might be.

Re: send-email: Net::SSLeay failure

[Samuel Lijin]

On Wed, Jun 7, 2017 at 8:26 PM, Liam Breck <l...@networkimprov.net> wrote:
> On Wed, Jun 7, 2017 at 5:06 PM, Samuel Lijin <sxli...@gmail.com> wrote:
>> On Wed, Jun 7, 2017 at 4:43 PM, Ævar Arnfjörð Bjarmason
>> <ava...@gmail.com> wrote:
>>> Ah, so you installed Net::SSLeay via CPAN, and then upgraded your Arch
>>> openssl, breaking the CPAN-built *.so object?
>>>
>>>> Agreed that send-email should be report errors properly. It's a rather
>>>> essential tool.
>>>
>>> Indeed, do you get a meaningful error if you apply my patch?
>>
>> FWIW I'm on Arch as well and am getting what I assume is a related error:
>>
>> SSLeay.c: loadable library and perl binaries are mismatched (got
>> handshake key 0xdb80080, needed 0xde00080)
>>
>> The last patch I sent out was on 5/23, and I just ran a system upgrade
>> today (only one I've done since then), which presumably is what
>> "broke" send-email. I used CPAN to install Net::SMTP::SSL,
>> Mime::Base64, and Authen::SASL; I assume that Net::SSLeay is installed
>> as a dependency of one of the three (presumably the first)?
>>
>> I know Net::SSLeay isn't controlled by Arch's package manager:
>>
>> $ perldoc -l Net::SSLeay
>> /usr/lib/perl5/site_perl/Net/SSLeay.pod
>> $ pacman -Qo /usr/lib/perl5/site_perl/Net/SSLeay.pod
>> error: No package owns /usr/lib/perl5/site_perl/Net/SSLeay.pod
>>
>> Ævar's patch unfortunately does not fix or change anything for me.
>
> Right because it attempts to log a diff issue.
>
>> Liam: when you say "uninstall" /usr/{lib,share}/perl5/site_perl/*, do
>> you just mean rm -rf?
>
> You need these new arch packages:
> perl-mime-tools perl-net-smtp-ssl perl-authen-sasl
>
> To uninstall the cpan stuff I did this in case I need to put anything back
>
> mkdir -p siteperl_uninstall/{lib,share}
> sudo mv /usr/share/perl5/site_perl/*  siteperl_uninstall/share/
> sudo mv /usr/lib/perl5/site_perl/*  siteperl_uninstall/lib/
>
> You could comment on this issue here
> https://bugs.archlinux.org/task/54334

Changed the subject line; my issue seems to be less related to yours
than I thought. I followed the steps you describe and I'm still
getting the mismatch error I mentioned above.

The closest I've gotten to figuring this out is that for some reason
SSLeay.so is unhappy:

$ ldd -r /usr/lib/perl5/vendor_perl/auto/Net/SSLeay/SSLeay.so
linux-vdso.so.1 (0x7ffc67fb)
libssl.so.1.1 => /usr/lib64/libssl.so.1.1 (0x7f891e58b000)
libcrypto.so.1.1 => /usr/lib64/libcrypto.so.1.1 (0x7f891e11)
libc.so.6 => /usr/lib64/libc.so.6 (0x7f891dd6b000)
libpthread.so.0 => /usr/lib64/libpthread.so.0 (0x7f891db4d000)
libdl.so.2 => /usr/lib64/libdl.so.2 (0x7f891d949000)
/usr/lib64/ld-linux-x86-64.so.2 (0x5598da604000)
undefined symbol: PL_thr_key
(/usr/lib/perl5/vendor_perl/auto/Net/SSLeay/SSLeay.so)
undefined symbol: Perl_sv_setnv_mg
(/usr/lib/perl5/vendor_perl/auto/Net/SSLeay/SSLea
[many more undefined symbol errors]

Ævar and anyone else familiar with Perl, any ideas? The best I can get
from the Arch forums is that this tends to occur when perl calls an
external lib built against a different version of perl, but I can't
tell if that's happening here.

Re: send-email: Net::SMTP::SSL failure

[Samuel Lijin]

On Wed, Jun 7, 2017 at 4:43 PM, Ævar Arnfjörð Bjarmason
 wrote:
> Ah, so you installed Net::SSLeay via CPAN, and then upgraded your Arch
> openssl, breaking the CPAN-built *.so object?
>
>> Agreed that send-email should be report errors properly. It's a rather
>> essential tool.
>
> Indeed, do you get a meaningful error if you apply my patch?

FWIW I'm on Arch as well and am getting what I assume is a related error:

SSLeay.c: loadable library and perl binaries are mismatched (got
handshake key 0xdb80080, needed 0xde00080)

The last patch I sent out was on 5/23, and I just ran a system upgrade
today (only one I've done since then), which presumably is what
"broke" send-email. I used CPAN to install Net::SMTP::SSL,
Mime::Base64, and Authen::SASL; I assume that Net::SSLeay is installed
as a dependency of one of the three (presumably the first)?

I know Net::SSLeay isn't controlled by Arch's package manager:

$ perldoc -l Net::SSLeay
/usr/lib/perl5/site_perl/Net/SSLeay.pod
$ pacman -Qo /usr/lib/perl5/site_perl/Net/SSLeay.pod
error: No package owns /usr/lib/perl5/site_perl/Net/SSLeay.pod

Ævar's patch unfortunately does not fix or change anything for me.

Liam: when you say "uninstall" /usr/{lib,share}/perl5/site_perl/*, do
you just mean rm -rf?

Re: pushing for a new hash, was Re: [PATCH 2/3] rebase: Add tests for console output

[Samuel Lijin]

On Tue, Jun 6, 2017 at 6:45 PM, Stefan Beller  wrote:
> On Tue, Jun 6, 2017 at 3:22 PM, Johannes Schindelin
>  wrote:
>>
>> 4) we still have the problem that there is no cryptography expert among
>> those who in the Git project are listened to
>
> I can assure you that Jonathan listened to crypto experts. It just did not
> happen on the mailing list, which is sad regarding openness and transparency.

In the interest of openness and transparency, perhaps a blue doc
should be put together to outline and document the hash function that
succeeds SHA1, and the rationales for doing so? It would, ideally,
cite (preferably by including, and not just linking to) any
discussions with crypto experts that have chimed in off-list (given
said experts' consent for any such communication to be publicized,
naturally).

If I'm not mistaken, the only such doc behind the transition right now
is the Git hash function transition document, which covers the
technical barriers to replacing SHA1, but not why we might choose X to
replace SHA1.

Re: What does this output of git supposed to mean ?

[Samuel Lijin]

On Mon, Jun 5, 2017 at 9:11 PM, brian m. carlson
 wrote:
> On Tue, Jun 06, 2017 at 10:00:12AM +0900, Junio C Hamano wrote:
>> Stefan Beller  writes:
>>
>> > On the subject: maybe we want to rename initial commit
>> > to root commit? (evil-me also thinks we could name it
>> > "parent-less commit", to reinforce what the lovely git man
>> > page generator tries to point at.)
>>
>> Is "initial" harder to understand than "root" or "parent-less"?
>
> I personally think that "Initial commit" is very straightforward.  If we
> get more inquiries about it, we could consider changing it, but seeing
> as this is the first question I've ever seen about it, I think most
> people understand it pretty well.

For what it's worth, I've never quite understood the "Initial commit"
message, because the repository is in a state where there are no
commits yet, not when HEAD is pointing to a root commit.

Re: git-gui ignores core.hooksPath

[Samuel Lijin]

On Fri, Jun 2, 2017 at 9:41 AM, Philipp Gortan  wrote:
> Hi git devs,
>
> First off, thanks for your awesome work!
>
> I've been unhappy for quite a while that I had to configure the hooks
> manually for each of my repos - until I found out recently that there is
> the core.hooksPath config variable that (when set globally) allows me to
> specify a hooks directory to be used for all my repositories.

OT but you may also want to look into using Git templates.

> Now I was happy - for a few minutes, until I tested this feature in
> git-gui, and realized that it doesn't work there.
>
> This seems to be caused by "proc githook_read", which says "set pchook
> [gitdir hooks $hook_name]" instead of querying "git config
> core.hooksPath" first - cf
> https://github.com/git/git/blob/2cc2e70264e0fcba04f9ef791d144bbc8b501206/git-gui/git-gui.sh#L627
>
> Would be great if this could get fixed...
>
> Thanks, Philipp
>

Re: preserve untracked cache, was Re: What's cooking in git.git (Jun 2017, #01; Thu, 1)

[Samuel Lijin]

On Thu, Jun 1, 2017 at 2:56 PM, Johannes Schindelin
 wrote:
> Hi Junio,
>
> On Thu, 1 Jun 2017, Junio C Hamano wrote:
>
>> * dt/unpack-save-untracked-cache-extension (2017-05-20) 1 commit
>>   (merged to 'next' on 2017-05-23 at 3196d093d6)
>>  + unpack-trees: preserve index extensions
>>
>>  When "git checkout", "git merge", etc. manipulates the in-core
>>  index, various pieces of information in the index extensions are
>>  discarded from the original state, as it is usually not the case
>>  that they are kept up-to-date and in-sync with the operation on the
>>  main index.  The untracked cache extension is copied across these
>>  operations now, which would speed up "git status" (as long as the
>>  cache is properly invalidated).
>
> It was my understanding that Ben's analysis conclusively proved that the
> patch as well as the test are sound, and Dave agreed.
>
> What is holding this topic up? Anything Ben or I can do to move this
> closer to `next` or even `master`?

It's in `next` right now (3196d093d6).

> Ciao,
> Dscho

Re: Coloring

[Samuel Lijin]

On Wed, May 31, 2017 at 5:10 PM, Irving Rabin <irv...@edmodo.com> wrote:
>
> Thanks Jeff, my problem has been resolved by Samuel Lijin.
> My terminal settings didn't set bold which remained white. I fixed it
> and my problem was gone!

Specifically, Irving's terminal rendered bold text as white. No bug here :)

> This issue is closed. Is there any way to retire it?

That's pretty much it.

> Irving Rabin
> Software Developer @Edmodo
> 408-242-1299
>
>
>
>
>
> On Wed, May 31, 2017 at 2:04 PM, Jeff King <p...@peff.net> wrote:
> > On Wed, May 31, 2017 at 11:33:31AM -0700, Irving Rabin wrote:
> >
> >> Specifically, if the field is supposed to be white, it doesn't mean it
> >> should be literally 0xFF. It should be the color that I have
> >> configured as White color for my console emulator.
> >>
> >> I like light-screen terminals, and I configure my ANSI colors in the
> >> way that they are clearly visible on the background and clearly
> >> distinct between themselves. In my terminal settings background is
> >> light-yellow, Black is black, Yellow is brown, Red is dark red,
> >> Magenta is purple and White is dark gray. I set it once and I can use
> >> it everywhere - all Unix commands work correctly, I can edit
> >> highlighted source code in Vim, and all my color settings are
> >> respected.
> >
> > Git outputs ANSI color codes, which are interpreted by your terminal.
> > You _can_ configure Git to send 24-bit color codes if your terminal
> > supports it, but by default it uses the traditional set of limited color
> > and attribute codes.
> >
> > What does running the following snippet in your shell look like?
> >
> > -- >8 --
> >
> > while read name code; do
> > printf '\033[%sm%s\033[m\n' "$code" "$name"
> > done <<-\EOF
> > normal
> > bold 1
> > red 31
> > green 32
> > yellow 33
> > blue 34
> > magenta 35
> > cyan 36
> > bold-red 1;31
> > bold-green 1;32
> > bold-yellow 1;33
> > bold-blue 1;34
> > bold-magenta 1;35
> > bold-cyan 1;36
> > EOF
> >
> > -- 8< --
> >
> > If any of the colors are not what you expect, is there a pattern? E.g.,
> > I wouldn't be surprised if "bold" shows up as bright white. In many
> > modern terminal emulators, the bold variants need to be configured
> > separately from the non-bold ones, and default to lighter variants of
> > their non-bold counterparts. The solution there would be to check your
> > terminal emulator config.
> >
> > If it does all look as you'd expect, try adding "| less -R" to the end of
> > the "done <<-\EOF" line. Most of Git's output goes through that pager
> > (though I _think_ it's mostly just passing through the ANSI codes, so it
> > wouldn't have any effect).
> >
> > -Peff

Re: Coloring

[Samuel Lijin]

On Wed, May 31, 2017 at 2:33 PM, Irving Rabin  wrote:
>
> Folks, I am reporting an issue with coloring of the output of Git
> commands, like status, diff, etc.
>
> Specifically, if the field is supposed to be white, it doesn't mean it
> should be literally 0xFF. It should be the color that I have
> configured as White color for my console emulator.
>
> I like light-screen terminals, and I configure my ANSI colors in the
> way that they are clearly visible on the background and clearly
> distinct between themselves. In my terminal settings background is
> light-yellow, Black is black, Yellow is brown, Red is dark red,
> Magenta is purple and White is dark gray. I set it once and I can use
> it everywhere - all Unix commands work correctly, I can edit
> highlighted source code in Vim, and all my color settings are
> respected.

Can you elaborate on how it is that you redefine your terminal color
scheme? There are multiple levels at which you can do that, which will
have some bearing on the answer.

> However Git behaves differently. When I run git diff, some of the
> output is literally white on light yellow background. It is like "we
> know what is White, so we ignore your settings". And it is quite
> irritating.
>
> Is there a way to make Git respect terminal settings and not to
> override them with absolute colors? If so, please advise. If not, then
> I guess it is a bug to fix, right?
>
> Thanks,
> Irving Rabin
> Software Developer @Edmodo
> 408-242-1299

Re: What's cooking in git.git (May 2017, #08; Mon, 29)

[Samuel Lijin]

On Mon, May 29, 2017 at 2:23 AM, Junio C Hamano  wrote:
> * sl/clean-d-ignored-fix (2017-05-24) 6 commits
>   (merged to 'next' on 2017-05-29 at 837c255ae8)
>  + clean: teach clean -d to preserve ignored paths
>  + dir: expose cmp_name() and check_contains()
>  + dir: hide untracked contents of untracked dirs
>  + dir: recurse into untracked dirs for ignored files
>  + t7061: status --ignored should search untracked dirs
>  + t7300: clean -d should skip dirs with ignored files
>
>  "git clean -d" used to clean directories that has ignored files,
>  even though the command should not lose ignored ones without "-x".
>  This has been corrected.
>
>  Will merge to 'master'.

I noticed the merge commit message (and branch name) don't make
mention of the changes to git status --ignored. I forgot to mention it
when the last cooking email when out, but was that intended?

I think at the very least, if the branch name isn't updated to reflect
that status --ignored is being changed, something like the following
should be appended to the merge commit message:

"git status --ignored" previously did not list ignored files in
untracked directories without -uall, contrary to the documented
behavior of the --ignored flag (that all ignored files would be
listed). This has also been corrected.

Re: [PATCH] doc: filter-branch does not require re-export of vars

[Samuel Lijin]

On Fri, May 26, 2017 at 2:37 PM, Jeff King  wrote:
>
> On Fri, May 26, 2017 at 07:36:54PM +0200, Andreas Heiduk wrote:
>
> > The function `set_ident` in `filter-branch` exported the variables
> > GIT_(AUTHOR|COMMITTER)_(NAME|EMAIL|DATE) at least since 6f6826c52b in 2007.
> > Therefore the filter scripts don't need to re-eport them again.
>
> Some old shells keep separate values for the internal and exporter
> versions of variables. I.e., this:
>
>   foo=one
>   export foo
>   foo=two
>
> would continue to export $foo as "one", even though it is "two" inside
> the script.
>
> However, I think POSIX mandates the behavior you'd expect. And the only
> shell I know that misbehaves in this way is Solaris /bin/sh, which we
> have already declared too broken to support.

Off-topic, but where is this explicitly documented?

> According to
>
>   
> https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/html_node/Limitations-of-Builtins.html#export
>
> it sounds like there are some other antique shells which may do the
> same (it doesn't cover this case explicitly, but the "coexist" cases it
> mentions are likely to behave in this way).
>
> At this point, I'd be inclined to remove the text as you suggest and
> either make a small note at the bottom of the page, or just omit it
> entirely and assume that anybody on an old non-POSIX shell can fend for
> themselves.
>
> -Peff

Re: [PATCH v6 6/6] clean: teach clean -d to preserve ignored paths

[Samuel Lijin]

On Wed, May 24, 2017 at 12:14 AM, Torsten Bögershausen  wrote:
>
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index d861f836a..937eb17b6 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -857,6 +857,38 @@ static void interactive_main_loop(void)
>> }
>>   }
>>   +static void correct_untracked_entries(struct dir_struct *dir)
>
> Looking what the function does, would
> drop_or_keep_untracked_entries()
> make more sense ?

To me, drop_or_keep_ implies that they're either all dropped or all
kept, nothing in between, which is why I went with correct_, to
indicate that the set of untracked entries in the dir_struct prior to
calling the method needs to be corrected.

[PATCH v6 6/6] clean: teach clean -d to preserve ignored paths

[Samuel Lijin]

There is an implicit assumption that a directory containing only
untracked and ignored paths should itself be considered untracked. This
makes sense in use cases where we're asking if a directory should be
added to the git database, but not when we're asking if a directory can
be safely removed from the working tree; as a result, clean -d would
assume that an "untracked" directory containing ignored paths could be
deleted, even though doing so would also remove the ignored paths.

To get around this, we teach clean -d to collect ignored paths and skip
an untracked directory if it contained an ignored path, instead just
removing the untracked contents thereof. To achieve this, cmd_clean()
has to collect all untracked contents of untracked directories, in
addition to all ignored paths, to determine which untracked dirs must be
skipped (because they contain ignored paths) and which ones should *not*
be skipped.

For this purpose, correct_untracked_entries() is introduced to prune a
given dir_struct of untracked entries containing ignored paths and those
untracked entries encompassed by the untracked entries which are not
pruned away.

A memory leak is also fixed in cmd_clean().

This also fixes the known breakage in t7300, since clean -d now skips
untracked directories containing ignored paths.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 builtin/clean.c  | 42 ++
 t/t7300-clean.sh |  2 +-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index d861f836a..937eb17b6 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -857,6 +857,38 @@ static void interactive_main_loop(void)
}
 }
 
+static void correct_untracked_entries(struct dir_struct *dir)
+{
+   int src, dst, ign;
+
+   for (src = dst = ign = 0; src < dir->nr; src++) {
+   /* skip paths in ignored[] that cannot be inside entries[src] */
+   while (ign < dir->ignored_nr &&
+  0 <= cmp_dir_entry(>entries[src], 
>ignored[ign]))
+   ign++;
+
+   if (ign < dir->ignored_nr &&
+   check_dir_entry_contains(dir->entries[src], 
dir->ignored[ign])) {
+   /* entries[src] contains an ignored path, so we drop it 
*/
+   free(dir->entries[src]);
+   } else {
+   struct dir_entry *ent = dir->entries[src++];
+
+   /* entries[src] does not contain an ignored path, so we 
keep it */
+   dir->entries[dst++] = ent;
+
+   /* then discard paths in entries[] contained inside 
entries[src] */
+   while (src < dir->nr &&
+  check_dir_entry_contains(ent, dir->entries[src]))
+   free(dir->entries[src++]);
+
+   /* compensate for the outer loop's loop control */
+   src--;
+   }
+   }
+   dir->nr = dst;
+}
+
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
int i, res;
@@ -916,6 +948,9 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
 
dir.flags |= DIR_SHOW_OTHER_DIRECTORIES;
 
+   if (remove_directories)
+   dir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;
+
if (read_cache() < 0)
die(_("index file corrupt"));
 
@@ -931,6 +966,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
   prefix, argv);
 
fill_directory(, );
+   correct_untracked_entries();
 
for (i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
@@ -958,6 +994,12 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
string_list_append(_list, rel);
}
 
+   for (i = 0; i < dir.nr; i++)
+   free(dir.entries[i]);
+
+   for (i = 0; i < dir.ignored_nr; i++)
+   free(dir.ignored[i]);
+
if (interactive && del_list.nr > 0)
interactive_main_loop();
 
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 3a2d709c2..7b36954d6 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,7 +653,7 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
-test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+test_expect_success 'git clean -d skips untracked dirs containing ignored 
files' '
echo /foo/bar >.gitignore &&
echo ignoreme >>.gitignore &&
rm -rf foo &&
-- 
2.13.0

[PATCH v6 3/6] dir: recurse into untracked dirs for ignored files

[Samuel Lijin]

We consider directories containing only untracked and ignored files to
be themselves untracked, which in the usual case means we don't have to
search these directories. This is problematic when we want to collect
ignored files with DIR_SHOW_IGNORED_TOO, though, so we teach
read_directory_recursive() to recurse into untracked directories to find
the ignored files they contain when DIR_SHOW_IGNORED_TOO is set. This
has the side effect of also collecting all untracked files in untracked
directories as well.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index f451bfa48..68cf6e47c 100644
--- a/dir.c
+++ b/dir.c
@@ -1784,7 +1784,10 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
dir_state = state;
 
/* recurse into subdir if instructed by treat_path */
-   if (state == path_recurse) {
+   if ((state == path_recurse) ||
+   ((state == path_untracked) &&
+(dir->flags & DIR_SHOW_IGNORED_TOO) &&
+(get_dtype(cdir.de, path.buf, path.len) == DT_DIR))) {
struct untracked_cache_dir *ud;
ud = lookup_untracked(dir->untracked, untracked,
  path.buf + baselen,
-- 
2.13.0

[PATCH v6 2/6] t7061: status --ignored should search untracked dirs

[Samuel Lijin]

Per eb8c5b87, `status --ignored` by design does not list ignored files
if they are in a directory which contains only ignored and untracked
files (which is itself considered to be untracked) without `-uall`. This
does not make sense for `--ignored`, which claims to "Show ignored files
as well."

Thus we revisit eb8c5b87 and decide that for such directories, `status
--ignored` will list the directory as untracked *and* list all ignored
files within said directory even without `-uall`.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7061-wtstatus-ignore.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index cdc0747bf..15e7592b6 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -9,9 +9,10 @@ cat >expected <<\EOF
 ?? actual
 ?? expected
 ?? untracked/
+!! untracked/ignored
 EOF
 
-test_expect_success 'status untracked directory with --ignored' '
+test_expect_failure 'status untracked directory with --ignored' '
echo "ignored" >.gitignore &&
mkdir untracked &&
: >untracked/ignored &&
@@ -20,7 +21,7 @@ test_expect_success 'status untracked directory with 
--ignored' '
test_cmp expected actual
 '
 
-test_expect_success 'same with gitignore starting with BOM' '
+test_expect_failure 'same with gitignore starting with BOM' '
printf "\357\273\277ignored\n" >.gitignore &&
mkdir -p untracked &&
: >untracked/ignored &&
-- 
2.13.0

[PATCH v6 4/6] dir: hide untracked contents of untracked dirs

[Samuel Lijin]

When we taught read_directory_recursive() to recurse into untracked
directories in search of ignored files given DIR_SHOW_IGNORED_TOO, that
had the side effect of teaching it to collect the untracked contents of
untracked directories. It doesn't always make sense to return these,
though (we do need them for `clean -d`), so we introduce a flag
(DIR_KEEP_UNTRACKED_CONTENTS) to control whether or not read_directory()
strips dir->entries of the untracked contents of untracked dirs.

We also introduce check_contains() to check if one dir_entry corresponds
to a path which contains the path corresponding to another dir_entry.

This also fixes known breakages in t7061, since status --ignored now
searches untracked directories for ignored files.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 Documentation/technical/api-directory-listing.txt |  6 +
 dir.c | 31 +++
 dir.h |  3 ++-
 t/t7061-wtstatus-ignore.sh|  4 +--
 4 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt 
b/Documentation/technical/api-directory-listing.txt
index 7f8e78d91..6c77b4920 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -33,6 +33,12 @@ The notable options are:
Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`
in addition to untracked files in `entries[]`.
 
+`DIR_KEEP_UNTRACKED_CONTENTS`:::
+
+   Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if this is set, 
the
+   untracked contents of untracked directories are also returned in
+   `entries[]`.
+
 `DIR_COLLECT_IGNORED`:::
 
Special mode for git-add. Return ignored files in `ignored[]` and
diff --git a/dir.c b/dir.c
index 68cf6e47c..ba5eadeda 100644
--- a/dir.c
+++ b/dir.c
@@ -1850,6 +1850,14 @@ static int cmp_name(const void *p1, const void *p2)
return name_compare(e1->name, e1->len, e2->name, e2->len);
 }
 
+/* check if *out lexically strictly contains *in */
+static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+{
+   return (out->len < in->len) &&
+   (out->name[out->len - 1] == '/') &&
+   !memcmp(out->name, in->name, out->len);
+}
+
 static int treat_leading_path(struct dir_struct *dir,
  const char *path, int len,
  const struct pathspec *pathspec)
@@ -2065,6 +2073,29 @@ int read_directory(struct dir_struct *dir, const char 
*path,
read_directory_recursive(dir, path, len, untracked, 0, 
pathspec);
QSORT(dir->entries, dir->nr, cmp_name);
QSORT(dir->ignored, dir->ignored_nr, cmp_name);
+
+   /* 
+* if DIR_SHOW_IGNORED_TOO, read_directory_recursive() will also pick
+* up untracked contents of untracked dirs; by default we discard these,
+* but given DIR_KEEP_UNTRACKED_CONTENTS we do not
+*/
+   if ((dir->flags & DIR_SHOW_IGNORED_TOO) &&
+!(dir->flags & DIR_KEEP_UNTRACKED_CONTENTS)) {
+   int i, j;
+
+   /* remove from dir->entries untracked contents of untracked 
dirs */
+   for (i = j = 0; j < dir->nr; j++) {
+   if (i && check_contains(dir->entries[i - 1], 
dir->entries[j])) {
+   free(dir->entries[j]);
+   dir->entries[j] = NULL;
+   } else {
+   dir->entries[i++] = dir->entries[j];
+   }
+   }
+
+   dir->nr = i;
+   }
+
if (dir->untracked) {
static struct trace_key trace_untracked_stats = 
TRACE_KEY_INIT(UNTRACKED_STATS);
trace_printf_key(_untracked_stats,
diff --git a/dir.h b/dir.h
index bf23a470a..650e54bdf 100644
--- a/dir.h
+++ b/dir.h
@@ -151,7 +151,8 @@ struct dir_struct {
DIR_NO_GITLINKS = 1<<3,
DIR_COLLECT_IGNORED = 1<<4,
DIR_SHOW_IGNORED_TOO = 1<<5,
-   DIR_COLLECT_KILLED_ONLY = 1<<6
+   DIR_COLLECT_KILLED_ONLY = 1<<6,
+   DIR_KEEP_UNTRACKED_CONTENTS = 1<<7
} flags;
struct dir_entry **entries;
struct dir_entry **ignored;
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 15e7592b6..fc6013ba3 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -12,7 +12,7 @@ cat >expected <<\EOF
 !! untracked/ignored
 EOF
 
-test_expect_failure 'status untracked directory with --ignored' '
+test_expect_success 'status untracked directory with --ignored'

[PATCH v6 5/6] dir: expose cmp_name() and check_contains()

[Samuel Lijin]

We want to use cmp_name() and check_contains() (which both compare
`struct dir_entry`s, the former in terms of the sort order, the latter
in terms of whether one lexically contains another) outside of dir.c,
so we have to (1) change their linkage and (2) rename them as
appropriate for the global namespace. The second is achieved by
renaming cmp_name() to cmp_dir_entry() and check_contains() to
check_dir_entry_contains().

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 11 ++-
 dir.h |  3 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index ba5eadeda..aae6d00b4 100644
--- a/dir.c
+++ b/dir.c
@@ -1842,7 +1842,7 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
return dir_state;
 }
 
-static int cmp_name(const void *p1, const void *p2)
+int cmp_dir_entry(const void *p1, const void *p2)
 {
const struct dir_entry *e1 = *(const struct dir_entry **)p1;
const struct dir_entry *e2 = *(const struct dir_entry **)p2;
@@ -1851,7 +1851,7 @@ static int cmp_name(const void *p1, const void *p2)
 }
 
 /* check if *out lexically strictly contains *in */
-static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+int check_dir_entry_contains(const struct dir_entry *out, const struct 
dir_entry *in)
 {
return (out->len < in->len) &&
(out->name[out->len - 1] == '/') &&
@@ -2071,8 +2071,8 @@ int read_directory(struct dir_struct *dir, const char 
*path,
dir->untracked = NULL;
if (!len || treat_leading_path(dir, path, len, pathspec))
read_directory_recursive(dir, path, len, untracked, 0, 
pathspec);
-   QSORT(dir->entries, dir->nr, cmp_name);
-   QSORT(dir->ignored, dir->ignored_nr, cmp_name);
+   QSORT(dir->entries, dir->nr, cmp_dir_entry);
+   QSORT(dir->ignored, dir->ignored_nr, cmp_dir_entry);
 
/* 
 * if DIR_SHOW_IGNORED_TOO, read_directory_recursive() will also pick
@@ -2085,7 +2085,8 @@ int read_directory(struct dir_struct *dir, const char 
*path,
 
/* remove from dir->entries untracked contents of untracked 
dirs */
for (i = j = 0; j < dir->nr; j++) {
-   if (i && check_contains(dir->entries[i - 1], 
dir->entries[j])) {
+   if (i &&
+   check_dir_entry_contains(dir->entries[i - 1], 
dir->entries[j])) {
free(dir->entries[j]);
dir->entries[j] = NULL;
} else {
diff --git a/dir.h b/dir.h
index 650e54bdf..edb5fda58 100644
--- a/dir.h
+++ b/dir.h
@@ -327,6 +327,9 @@ static inline int dir_path_match(const struct dir_entry 
*ent,
  has_trailing_dir);
 }
 
+int cmp_dir_entry(const void *p1, const void *p2);
+int check_dir_entry_contains(const struct dir_entry *out, const struct 
dir_entry *in);
+
 void untracked_cache_invalidate_path(struct index_state *, const char *);
 void untracked_cache_remove_from_index(struct index_state *, const char *);
 void untracked_cache_add_to_index(struct index_state *, const char *);
-- 
2.13.0

[PATCH v6 1/6] t7300: clean -d should skip dirs with ignored files

[Samuel Lijin]

If git sees a directory which contains only untracked and ignored
files, clean -d should not remove that directory. It was recently
discovered that this is *not* true of git clean -d, and it's possible
that this has never worked correctly; this test and its accompanying
patch series aims to fix that.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7300-clean.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index b89fd2a6a..3a2d709c2 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,4 +653,20 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
+test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+   echo /foo/bar >.gitignore &&
+   echo ignoreme >>.gitignore &&
+   rm -rf foo &&
+   mkdir -p foo/a/aa/aaa foo/b/bb/bbb &&
+   touch foo/bar foo/baz foo/a/aa/ignoreme foo/b/ignoreme foo/b/bb/1 
foo/b/bb/2 &&
+   git clean -df &&
+   test_path_is_dir foo &&
+   test_path_is_file foo/bar &&
+   test_path_is_missing foo/baz &&
+   test_path_is_file foo/a/aa/ignoreme &&
+   test_path_is_missing foo/a/aa/aaa &&
+   test_path_is_file foo/b/ignoreme &&
+   test_path_is_missing foo/b/bb
+'
+
 test_done
-- 
2.13.0

[PATCH v6 0/6] Fix clean -d and status --ignored

[Samuel Lijin]

Messed up on 6/6 in v5, forgot to include changes from earlier versions (karma
for not running tests before I send-email'd the patch series).

Samuel Lijin (6):
  t7300: clean -d should skip dirs with ignored files
  t7061: status --ignored should search untracked dirs
  dir: recurse into untracked dirs for ignored files
  dir: hide untracked contents of untracked dirs
  dir: expose cmp_name() and check_contains()
  clean: teach clean -d to preserve ignored paths

 Documentation/technical/api-directory-listing.txt |  6 
 builtin/clean.c   | 42 ++
 dir.c | 43 ---
 dir.h |  6 +++-
 t/t7061-wtstatus-ignore.sh|  1 +
 t/t7300-clean.sh  | 16 +
 6 files changed, 109 insertions(+), 5 deletions(-)

-- 
2.13.0

Re: [PATCH v5 6/6] clean: teach clean -d to preserve ignored paths

[Samuel Lijin]

On Tue, May 23, 2017 at 8:52 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Samuel Lijin <sxli...@gmail.com> writes:
>
>> @@ -931,6 +961,7 @@ int cmd_clean(int argc, const char **argv, const char 
>> *prefix)
>>  prefix, argv);
>>
>>   fill_directory(, );
>> + correct_untracked_entries();
>>
>>   for (i = 0; i < dir.nr; i++) {
>>   struct dir_entry *ent = dir.entries[i];
>
> You used to set SHOW_IGNORED_TOO and KEEP_UNTRACKED_CONTENTS in
> dir.flags early in the function, and then free dir.entries[] and
> dir.ignored[] after we are done.  They are gone in this version.
>
> Intended?

Embarrassingly, no. Rerolling.

>> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
>> index 3a2d709c2..7b36954d6 100755
>> --- a/t/t7300-clean.sh
>> +++ b/t/t7300-clean.sh
>> @@ -653,7 +653,7 @@ test_expect_success 'git clean -d respects pathspecs 
>> (pathspec is prefix of dir)
>>   test_path_is_dir foobar
>>  '
>>
>> -test_expect_failure 'git clean -d skips untracked dirs containing ignored 
>> files' '
>> +test_expect_success 'git clean -d skips untracked dirs containing ignored 
>> files' '
>>   echo /foo/bar >.gitignore &&
>>   echo ignoreme >>.gitignore &&
>>   rm -rf foo &&

[PATCH v5 4/6] dir: hide untracked contents of untracked dirs

[Samuel Lijin]

When we taught read_directory_recursive() to recurse into untracked
directories in search of ignored files given DIR_SHOW_IGNORED_TOO, that
had the side effect of teaching it to collect the untracked contents of
untracked directories. It doesn't always make sense to return these,
though (we do need them for `clean -d`), so we introduce a flag
(DIR_KEEP_UNTRACKED_CONTENTS) to control whether or not read_directory()
strips dir->entries of the untracked contents of untracked dirs.

We also introduce check_contains() to check if one dir_entry corresponds
to a path which contains the path corresponding to another dir_entry.

This also fixes known breakages in t7061, since status --ignored now
searches untracked directories for ignored files.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 Documentation/technical/api-directory-listing.txt |  6 +
 dir.c | 31 +++
 dir.h |  3 ++-
 t/t7061-wtstatus-ignore.sh|  4 +--
 4 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt 
b/Documentation/technical/api-directory-listing.txt
index 7f8e78d91..6c77b4920 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -33,6 +33,12 @@ The notable options are:
Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`
in addition to untracked files in `entries[]`.
 
+`DIR_KEEP_UNTRACKED_CONTENTS`:::
+
+   Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if this is set, 
the
+   untracked contents of untracked directories are also returned in
+   `entries[]`.
+
 `DIR_COLLECT_IGNORED`:::
 
Special mode for git-add. Return ignored files in `ignored[]` and
diff --git a/dir.c b/dir.c
index 68cf6e47c..ba5eadeda 100644
--- a/dir.c
+++ b/dir.c
@@ -1850,6 +1850,14 @@ static int cmp_name(const void *p1, const void *p2)
return name_compare(e1->name, e1->len, e2->name, e2->len);
 }
 
+/* check if *out lexically strictly contains *in */
+static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+{
+   return (out->len < in->len) &&
+   (out->name[out->len - 1] == '/') &&
+   !memcmp(out->name, in->name, out->len);
+}
+
 static int treat_leading_path(struct dir_struct *dir,
  const char *path, int len,
  const struct pathspec *pathspec)
@@ -2065,6 +2073,29 @@ int read_directory(struct dir_struct *dir, const char 
*path,
read_directory_recursive(dir, path, len, untracked, 0, 
pathspec);
QSORT(dir->entries, dir->nr, cmp_name);
QSORT(dir->ignored, dir->ignored_nr, cmp_name);
+
+   /* 
+* if DIR_SHOW_IGNORED_TOO, read_directory_recursive() will also pick
+* up untracked contents of untracked dirs; by default we discard these,
+* but given DIR_KEEP_UNTRACKED_CONTENTS we do not
+*/
+   if ((dir->flags & DIR_SHOW_IGNORED_TOO) &&
+!(dir->flags & DIR_KEEP_UNTRACKED_CONTENTS)) {
+   int i, j;
+
+   /* remove from dir->entries untracked contents of untracked 
dirs */
+   for (i = j = 0; j < dir->nr; j++) {
+   if (i && check_contains(dir->entries[i - 1], 
dir->entries[j])) {
+   free(dir->entries[j]);
+   dir->entries[j] = NULL;
+   } else {
+   dir->entries[i++] = dir->entries[j];
+   }
+   }
+
+   dir->nr = i;
+   }
+
if (dir->untracked) {
static struct trace_key trace_untracked_stats = 
TRACE_KEY_INIT(UNTRACKED_STATS);
trace_printf_key(_untracked_stats,
diff --git a/dir.h b/dir.h
index bf23a470a..650e54bdf 100644
--- a/dir.h
+++ b/dir.h
@@ -151,7 +151,8 @@ struct dir_struct {
DIR_NO_GITLINKS = 1<<3,
DIR_COLLECT_IGNORED = 1<<4,
DIR_SHOW_IGNORED_TOO = 1<<5,
-   DIR_COLLECT_KILLED_ONLY = 1<<6
+   DIR_COLLECT_KILLED_ONLY = 1<<6,
+   DIR_KEEP_UNTRACKED_CONTENTS = 1<<7
} flags;
struct dir_entry **entries;
struct dir_entry **ignored;
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 15e7592b6..fc6013ba3 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -12,7 +12,7 @@ cat >expected <<\EOF
 !! untracked/ignored
 EOF
 
-test_expect_failure 'status untracked directory with --ignored' '
+test_expect_success 'status untracked directory with --ignored'

[PATCH v5 5/6] dir: expose cmp_name() and check_contains()

[Samuel Lijin]

We want to use cmp_name() and check_contains() (which both compare
`struct dir_entry`s, the former in terms of the sort order, the latter
in terms of whether one lexically contains another) outside of dir.c,
so we have to (1) change their linkage and (2) rename them as
appropriate for the global namespace. The second is achieved by
renaming cmp_name() to cmp_dir_entry() and check_contains() to
check_dir_entry_contains().

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 11 ++-
 dir.h |  3 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index ba5eadeda..aae6d00b4 100644
--- a/dir.c
+++ b/dir.c
@@ -1842,7 +1842,7 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
return dir_state;
 }
 
-static int cmp_name(const void *p1, const void *p2)
+int cmp_dir_entry(const void *p1, const void *p2)
 {
const struct dir_entry *e1 = *(const struct dir_entry **)p1;
const struct dir_entry *e2 = *(const struct dir_entry **)p2;
@@ -1851,7 +1851,7 @@ static int cmp_name(const void *p1, const void *p2)
 }
 
 /* check if *out lexically strictly contains *in */
-static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+int check_dir_entry_contains(const struct dir_entry *out, const struct 
dir_entry *in)
 {
return (out->len < in->len) &&
(out->name[out->len - 1] == '/') &&
@@ -2071,8 +2071,8 @@ int read_directory(struct dir_struct *dir, const char 
*path,
dir->untracked = NULL;
if (!len || treat_leading_path(dir, path, len, pathspec))
read_directory_recursive(dir, path, len, untracked, 0, 
pathspec);
-   QSORT(dir->entries, dir->nr, cmp_name);
-   QSORT(dir->ignored, dir->ignored_nr, cmp_name);
+   QSORT(dir->entries, dir->nr, cmp_dir_entry);
+   QSORT(dir->ignored, dir->ignored_nr, cmp_dir_entry);
 
/* 
 * if DIR_SHOW_IGNORED_TOO, read_directory_recursive() will also pick
@@ -2085,7 +2085,8 @@ int read_directory(struct dir_struct *dir, const char 
*path,
 
/* remove from dir->entries untracked contents of untracked 
dirs */
for (i = j = 0; j < dir->nr; j++) {
-   if (i && check_contains(dir->entries[i - 1], 
dir->entries[j])) {
+   if (i &&
+   check_dir_entry_contains(dir->entries[i - 1], 
dir->entries[j])) {
free(dir->entries[j]);
dir->entries[j] = NULL;
} else {
diff --git a/dir.h b/dir.h
index 650e54bdf..edb5fda58 100644
--- a/dir.h
+++ b/dir.h
@@ -327,6 +327,9 @@ static inline int dir_path_match(const struct dir_entry 
*ent,
  has_trailing_dir);
 }
 
+int cmp_dir_entry(const void *p1, const void *p2);
+int check_dir_entry_contains(const struct dir_entry *out, const struct 
dir_entry *in);
+
 void untracked_cache_invalidate_path(struct index_state *, const char *);
 void untracked_cache_remove_from_index(struct index_state *, const char *);
 void untracked_cache_add_to_index(struct index_state *, const char *);
-- 
2.13.0

[PATCH v5 0/6] Fix clean -d and status --ignored

[Samuel Lijin]

Incorporates latest round of feedback from Junio about how to best structure
the changes to cmd_clean() for maintainability.

Samuel Lijin (6):
  t7300: clean -d should skip dirs with ignored files
  t7061: status --ignored should search untracked dirs
  dir: recurse into untracked dirs for ignored files
  dir: hide untracked contents of untracked dirs
  dir: expose cmp_name() and check_contains()
  clean: teach clean -d to preserve ignored paths

 Documentation/technical/api-directory-listing.txt |  6 
 builtin/clean.c   | 31 
 dir.c | 43 ---
 dir.h |  6 +++-
 t/t7061-wtstatus-ignore.sh|  1 +
 t/t7300-clean.sh  | 16 +
 6 files changed, 98 insertions(+), 5 deletions(-)

-- 
2.13.0

[PATCH v5 3/6] dir: recurse into untracked dirs for ignored files

[Samuel Lijin]

We consider directories containing only untracked and ignored files to
be themselves untracked, which in the usual case means we don't have to
search these directories. This is problematic when we want to collect
ignored files with DIR_SHOW_IGNORED_TOO, though, so we teach
read_directory_recursive() to recurse into untracked directories to find
the ignored files they contain when DIR_SHOW_IGNORED_TOO is set. This
has the side effect of also collecting all untracked files in untracked
directories as well.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index f451bfa48..68cf6e47c 100644
--- a/dir.c
+++ b/dir.c
@@ -1784,7 +1784,10 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
dir_state = state;
 
/* recurse into subdir if instructed by treat_path */
-   if (state == path_recurse) {
+   if ((state == path_recurse) ||
+   ((state == path_untracked) &&
+(dir->flags & DIR_SHOW_IGNORED_TOO) &&
+(get_dtype(cdir.de, path.buf, path.len) == DT_DIR))) {
struct untracked_cache_dir *ud;
ud = lookup_untracked(dir->untracked, untracked,
  path.buf + baselen,
-- 
2.13.0

[PATCH v5 1/6] t7300: clean -d should skip dirs with ignored files

[Samuel Lijin]

If git sees a directory which contains only untracked and ignored
files, clean -d should not remove that directory. It was recently
discovered that this is *not* true of git clean -d, and it's possible
that this has never worked correctly; this test and its accompanying
patch series aims to fix that.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7300-clean.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index b89fd2a6a..3a2d709c2 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,4 +653,20 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
+test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+   echo /foo/bar >.gitignore &&
+   echo ignoreme >>.gitignore &&
+   rm -rf foo &&
+   mkdir -p foo/a/aa/aaa foo/b/bb/bbb &&
+   touch foo/bar foo/baz foo/a/aa/ignoreme foo/b/ignoreme foo/b/bb/1 
foo/b/bb/2 &&
+   git clean -df &&
+   test_path_is_dir foo &&
+   test_path_is_file foo/bar &&
+   test_path_is_missing foo/baz &&
+   test_path_is_file foo/a/aa/ignoreme &&
+   test_path_is_missing foo/a/aa/aaa &&
+   test_path_is_file foo/b/ignoreme &&
+   test_path_is_missing foo/b/bb
+'
+
 test_done
-- 
2.13.0

[PATCH v5 6/6] clean: teach clean -d to preserve ignored paths

[Samuel Lijin]

There is an implicit assumption that a directory containing only
untracked and ignored paths should itself be considered untracked. This
makes sense in use cases where we're asking if a directory should be
added to the git database, but not when we're asking if a directory can
be safely removed from the working tree; as a result, clean -d would
assume that an "untracked" directory containing ignored paths could be
deleted, even though doing so would also remove the ignored paths.

To get around this, we teach clean -d to collect ignored paths and skip
an untracked directory if it contained an ignored path, instead just
removing the untracked contents thereof. To achieve this, cmd_clean()
has to collect all untracked contents of untracked directories, in
addition to all ignored paths, to determine which untracked dirs must be
skipped (because they contain ignored paths) and which ones should *not*
be skipped.

For this purpose, correct_untracked_entries() is introduced to prune a
given dir_struct of untracked entries containing ignored paths and those
untracked entries encompassed by the untracked entries which are not
pruned away.

This also fixes the known breakage in t7300, since clean -d now skips
untracked directories containing ignored paths.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 builtin/clean.c  | 31 +++
 t/t7300-clean.sh |  2 +-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index d861f836a..45dbdcd18 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -857,6 +857,36 @@ static void interactive_main_loop(void)
}
 }
 
+static void correct_untracked_entries(struct dir_struct *dir)
+{
+   int src, dst, ign;
+
+   for (src = dst = ign = 0; src < dir->nr; src++) {
+   /* skip paths in ignored[] that cannot be inside entries[src] */
+   while (ign < dir->ignored_nr &&
+  0 <= cmp_dir_entry(>entries[src], 
>ignored[ign]))
+   ign++;
+
+   if (ign < dir->ignored_nr &&
+   check_dir_entry_contains(dir->entries[src], 
dir->ignored[ign])) {
+   /* entries[src] contains an ignored path, so we drop it 
*/
+   free(dir->entries[src]);
+   } else {
+   /* entries[src] does not contain an ignored path, so we 
keep it */
+   dir->entries[dst++] = dir->entries[src++];
+
+   /* then discard paths in entries[] contained inside 
entries[src] */
+   while (src < dir->nr &&
+  check_dir_entry_contains(ent, dir->entries[src]))
+   free(dir->entries[src++]);
+
+   /* compensate for the outer loop's loop control */
+   src--;
+   }
+   }
+   dir->nr = dst;
+}
+
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
int i, res;
@@ -931,6 +961,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
   prefix, argv);
 
fill_directory(, );
+   correct_untracked_entries();
 
for (i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 3a2d709c2..7b36954d6 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,7 +653,7 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
-test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+test_expect_success 'git clean -d skips untracked dirs containing ignored 
files' '
echo /foo/bar >.gitignore &&
echo ignoreme >>.gitignore &&
rm -rf foo &&
-- 
2.13.0

[PATCH v5 2/6] t7061: status --ignored should search untracked dirs

[Samuel Lijin]

Per eb8c5b87, `status --ignored` by design does not list ignored files
if they are in a directory which contains only ignored and untracked
files (which is itself considered to be untracked) without `-uall`. This
does not make sense for `--ignored`, which claims to "Show ignored files
as well."

Thus we revisit eb8c5b87 and decide that for such directories, `status
--ignored` will list the directory as untracked *and* list all ignored
files within said directory even without `-uall`.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7061-wtstatus-ignore.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index cdc0747bf..15e7592b6 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -9,9 +9,10 @@ cat >expected <<\EOF
 ?? actual
 ?? expected
 ?? untracked/
+!! untracked/ignored
 EOF
 
-test_expect_success 'status untracked directory with --ignored' '
+test_expect_failure 'status untracked directory with --ignored' '
echo "ignored" >.gitignore &&
mkdir untracked &&
: >untracked/ignored &&
@@ -20,7 +21,7 @@ test_expect_success 'status untracked directory with 
--ignored' '
test_cmp expected actual
 '
 
-test_expect_success 'same with gitignore starting with BOM' '
+test_expect_failure 'same with gitignore starting with BOM' '
printf "\357\273\277ignored\n" >.gitignore &&
mkdir -p untracked &&
: >untracked/ignored &&
-- 
2.13.0

Re: [Bug] cloning a repository with a default MASTER branch tries to check out the master branch

[Samuel Lijin]

For some reason the repo on GH does not have a HEAD pointer:

$ git ls-remote https://github.com/passcod/UPPERCASE-NPM.git
efc7dbfd6ca155d5d19ce67eb98603896062f35arefs/heads/MASTER
e60ea8e6ec45ec45ff44ac8939cb4105b16477darefs/pull/1/head
f35a73dcb151d336dc3d30c9a2c7423ecdb7bd1crefs/pull/2/head
0d9b3a1268ff39350e04a7183af0add912b686e6refs/tags/V1.0.0
efc7dbfd6ca155d5d19ce67eb98603896062f35arefs/tags/V1.0.1

I'm not sure how you managed to do that, since GH rejects attempts to
delete the current branch, but I believe if you set the default branch
to MASTER it will work correctly.

On Mon, May 22, 2017 at 5:42 PM, Félix Saparelli  wrote:
> Hi,
>
> I created a git repository that, for joke reasons, has a single branch
> called MASTER (in uppercase). Upon cloning this repo, git attempts to
> checkout the master branch (in lowercase), which does not exist.
> Checking out the MASTER branch manually afterwards works.
>
> $ git clone g...@github.com:passcod/UPPERCASE-NPM.git
> Cloning into 'UPPERCASE-NPM'...
> remote: Counting objects: 14, done.
> remote: Compressing objects: 100% (11/11), done.
> remote: Total 14 (delta 3), reused 14 (delta 3), pack-reused 0
> Receiving objects: 100% (14/14), done.
> Resolving deltas: 100% (3/3), done.
> warning: remote HEAD refers to nonexistent ref, unable to checkout.
>
> $ cd UPPERCASE-NPM
> $ ls -a
> . .. .git
> $ git branch
> $ git checkout MASTER
> Branch MASTER set up to track remote branch MASTER from origin.
> Switched to a new branch 'MASTER'
> $ ls -a
> . .. .git NPM package.json README
> $ git branch
> * MASTER
>
> Some platform information:
>
> $ git version
> git version 2.12.2
>
> $ uname -a
> Linux felix-probook 4.10.13-1-ARCH #1 SMP PREEMPT Thu Apr 27 12:15:09
> CEST 2017 x86_64 GNU/Linux
>
> Git was installed from the default Arch Linux package.
>
> Thanks,
> Félix
>

Re: [PATCH v4 6/6] clean: teach clean -d to skip dirs containing ignored files

[Samuel Lijin]

On Mon, May 22, 2017 at 2:17 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Samuel Lijin <sxli...@gmail.com> writes:
>
>>> By the way, instead of putting NULL, it may be easier to follow if
>>> you used two pointers, src and dst, into dir.entries[], just like
>>> you did in your latest version of [PATCH 4/6].  That way, you do not
>>> have to change anything in the later loop that walks over elements
>>> in the dir.entries[] array.  It would also help the logic easier to
>>> follow if the above loop were its own helper function.
>>
>> Agreed on the helper function. On the src-dst thing: I considered it,
>> but I figured another O(n) set of array moves was unnecessary. I guess
>> this is one of those cases where premature optimization doesn't make
>> sense?
>
> I actually did not mean to give the variables more descriptive names
> and preserve the original 'main loop' (namely, not adding the "skip
> if NULL" which would never happen in normal case where "-d" is not
> used without "-x") as "optimization", whether it is premature or
> not.  My suggestions were purely from "wouldn't the resulting code
> easier to follow and understand, leading to fewer bugs in the
> future?" point of view.
>
> As I said, I am undecided if the result is easier to follow than
> your version ;-)

I think I'll defer to your patch: I do agree that your version is
easier to follow and understand. Should I reroll just this patch and
its commit message, or would you prefer to handle that in the queuing
yourself?

Re: [PATCH v4 6/6] clean: teach clean -d to skip dirs containing ignored files

[Samuel Lijin]

On Mon, May 22, 2017 at 12:48 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Samuel Lijin <sxli...@gmail.com> writes:
>
>> + for (j = i = 0; i < dir.nr;) {
>> + for (;
>> +  j < dir.ignored_nr &&
>> +0 <= cmp_dir_entry([i], [j]);
>> +  j++);
>> +
>> + if ((j < dir.ignored_nr) &&
>> + check_dir_entry_contains(dir.entries[i], 
>> dir.ignored[j])) {
>> + /* skip any dir.entries which contains a dir.ignored */
>> + free(dir.entries[i]);
>> + dir.entries[i++] = NULL;
>> + } else {
>> + /* prune the contents of a dir.entries which will be 
>> removed */
>> + struct dir_entry *ent = dir.entries[i++];
>> + for (;
>> +  i < dir.nr &&
>> +check_dir_entry_contains(ent, dir.entries[i]);
>> +  i++) {
>> + free(dir.entries[i]);
>> + dir.entries[i] = NULL;
>> + }
>> + }
>> + }
>
> The second loop in the else clause is a bit tricky, and the comment
> "which will be removed" is not all that helpful to explain why the
> loop is there.
>
> But I think the code is correct.  Here is how I understood it.
>
> While looking at dir.entries[i], the code noticed that nothing
> in that directory is ignored.  But entries in dir.entries[] that
> come later may be contained in dir.entries[i] and we just want
> to show the top-level untracked one (e.g. "a/" and "a/b/" were
> in entries[], there is nothing in "a/", so naturally there is
> nothing in "a/b/", but we do not want to bother showing
> both---showing "a/" alone saying "the entire a/ is untracked" is
> what we want).

Yep, that's the gist of it.

More specifically: the contents of untracked dirs have to be picked up
so that clean -d can distinguish between purely untracked dirs and
untracked dirs which also contain ignored files. In the case of a
purely untracked dir, clean -d can remove the dir itself, and should
just skip over all the dir's contents; in the case of a mixed
untracked dir, clean -d should not remove the dir itself, just the
untracked contents.

> We may want to have a test to ensure "a/b/" is indeed omitted in
> such a situation from the output, though.

Is there a reason to ensure specifically a/b/ is tested, or are the
current tests, which do cover the a/b (i.e. where a/b is a file, not
where a/b/ is a dir) case, insufficient for some reason?

> By the way, instead of putting NULL, it may be easier to follow if
> you used two pointers, src and dst, into dir.entries[], just like
> you did in your latest version of [PATCH 4/6].  That way, you do not
> have to change anything in the later loop that walks over elements
> in the dir.entries[] array.  It would also help the logic easier to
> follow if the above loop were its own helper function.

Agreed on the helper function. On the src-dst thing: I considered it,
but I figured another O(n) set of array moves was unnecessary. I guess
this is one of those cases where premature optimization doesn't make
sense?

> Putting them all together, here is what I came up with that can be
> squashed into your patch.  I am undecided myself if this is easier
> to follow than your version, but it seems to pass your test ;-)
>
> Thanks.
>
>  builtin/clean.c | 70 
> ++---
>  1 file changed, 42 insertions(+), 28 deletions(-)
>
> diff --git a/builtin/clean.c b/builtin/clean.c
> index dd3308a447..c8712e7ac8 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -851,9 +851,49 @@ static void interactive_main_loop(void)
> }
>  }
>
> +static void simplify_untracked(struct dir_struct *dir)
> +{
> +   int src, dst, ign;
> +
> +   for (src = dst = ign = 0; src < dir->nr; src++) {
> +   /*
> +* Skip entries in ignored[] that cannot be inside
> +* entries[src]
> +*/
> +   while (ign < dir->ignored_nr &&
> +  0 <= cmp_dir_entry(>entries[src], 
> >ignored[ign]))
> +   ign++;
> +
> +   if (dir->ignored_nr <= ign ||
> +   !check_dir_entry_contains(dir->entries[src], 
> dir->ign

Re: [PATCH v2 1/2] refs: Add for_each_worktree_ref for iterating over all worktree HEADs

[Samuel Lijin]

On Thu, May 18, 2017 at 5:40 AM, Simon Ruderich  wrote:
> On Wed, May 17, 2017 at 06:45:31PM -0700, Manish Goregaokar wrote:
>> Hm, my invocation of git-send-email keeps getting the threading wrong.
>> Is there a recommended set of arguments to the command?
>
> The threading looks fine here (for both cases where you mentioned
> it being wrong). Why do you think it's wrong? How does it look on
> your end?

If you're on gmail (as Manish and I both are) patches in a subsequent
version will be threaded (wrongly) against "earlier" versions of the
patch. So if you have patch series A0, A1, A2, A3 and new version B0,
B1, B2, if you thread them as

A0
- A1
- A2
- A3
- B0
  - B1
  - B2

gmail will show them in your inbox as

A0
- B0

A1
- B1

A2

A3
- B2

Depending on whatever heuristics they use to match up "threads",
presumably because most emails aren't threaded correctly.

[PATCH v4 0/6] Fix clean -d and status --ignored

[Samuel Lijin]

Incorporates all of Junio's feedback on v4:

* squashes test expect_failure -> expect_success flips into the commits that
  fix them
* the O(n^2) pruning loop added for DIR_KEEP_UNTRACKED_CONTENTS is now O(n)
* the logic in cmd_clean() that keeps the contents of an untracked dir from the
  del_list is now hoisted into a separate loop

Also includes the following:

* expands the test I add in t7300 to check clean -d, for better coverage of the
  pruning logic in cmd_clean() (the logic that tells clean -d to not remove a/b
  and a/c if it's already removing a/)
* documents DIR_KEEP_UNTRACKED_CONTENTS in the corresponding technical API doc

Samuel Lijin (6):
  t7300: clean -d should skip dirs with ignored files
  t7061: status --ignored should search untracked dirs
  dir: recurse into untracked dirs for ignored files
  dir: hide untracked contents of untracked dirs
  dir: expose cmp_name() and check_contains()
  clean: teach clean -d to skip dirs containing ignored files

 Documentation/technical/api-directory-listing.txt |  6 
 builtin/clean.c   | 38 +++-
 dir.c | 42 ---
 dir.h |  6 +++-
 t/t7061-wtstatus-ignore.sh|  1 +
 t/t7300-clean.sh  | 16 +
 6 files changed, 103 insertions(+), 6 deletions(-)

-- 
2.13.0

[PATCH v4 4/6] dir: hide untracked contents of untracked dirs

[Samuel Lijin]

When we taught read_directory_recursive() to recurse into untracked
directories in search of ignored files given DIR_SHOW_IGNORED_TOO, that
had the side effect of teaching it to collect the untracked contents of
untracked directories. It doesn't always make sense to return these,
though (we do need them for `clean -d`), so we introduce a flag
(DIR_KEEP_UNTRACKED_CONTENTS) to control whether or not read_directory()
strips dir->entries of the untracked contents of untracked dirs.

We also introduce check_contains() to check if one dir_entry corresponds
to a path which contains the path corresponding to another dir_entry.

This also fixes known breakages in t7061, since status --ignored now
searches untracked directories for ignored files.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 Documentation/technical/api-directory-listing.txt |  6 +
 dir.c | 30 +++
 dir.h |  3 ++-
 t/t7061-wtstatus-ignore.sh|  4 +--
 4 files changed, 40 insertions(+), 3 deletions(-)

diff --git a/Documentation/technical/api-directory-listing.txt 
b/Documentation/technical/api-directory-listing.txt
index 7f8e78d91..6c77b4920 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -33,6 +33,12 @@ The notable options are:
Similar to `DIR_SHOW_IGNORED`, but return ignored files in `ignored[]`
in addition to untracked files in `entries[]`.
 
+`DIR_KEEP_UNTRACKED_CONTENTS`:::
+
+   Only has meaning if `DIR_SHOW_IGNORED_TOO` is also set; if this is set, 
the
+   untracked contents of untracked directories are also returned in
+   `entries[]`.
+
 `DIR_COLLECT_IGNORED`:::
 
Special mode for git-add. Return ignored files in `ignored[]` and
diff --git a/dir.c b/dir.c
index 68cf6e47c..4b0a99323 100644
--- a/dir.c
+++ b/dir.c
@@ -1850,6 +1850,14 @@ static int cmp_name(const void *p1, const void *p2)
return name_compare(e1->name, e1->len, e2->name, e2->len);
 }
 
+/* check if *out lexically strictly contains *in */
+static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+{
+   return (out->len < in->len) &&
+   (out->name[out->len - 1] == '/') &&
+   !memcmp(out->name, in->name, out->len);
+}
+
 static int treat_leading_path(struct dir_struct *dir,
  const char *path, int len,
  const struct pathspec *pathspec)
@@ -2065,6 +2073,28 @@ int read_directory(struct dir_struct *dir, const char 
*path,
read_directory_recursive(dir, path, len, untracked, 0, 
pathspec);
QSORT(dir->entries, dir->nr, cmp_name);
QSORT(dir->ignored, dir->ignored_nr, cmp_name);
+
+   /* if DIR_SHOW_IGNORED_TOO, read_directory_recursive() will also pick
+* up untracked contents of untracked dirs; by default we discard these,
+* but given DIR_KEEP_UNTRACKED_CONTENTS we do not
+*/
+   if ((dir->flags & DIR_SHOW_IGNORED_TOO) &&
+!(dir->flags & DIR_KEEP_UNTRACKED_CONTENTS)) {
+   int i, j;
+
+   /* remove from dir->entries untracked contents of untracked 
dirs */
+   for (i = j = 0; j < dir->nr; j++) {
+   if (i && check_contains(dir->entries[i - 1], 
dir->entries[j])) {
+   free(dir->entries[j]);
+   dir->entries[j] = NULL;
+   } else {
+   dir->entries[i++] = dir->entries[j];
+   }
+   }
+
+   dir->nr = i;
+   }
+
if (dir->untracked) {
static struct trace_key trace_untracked_stats = 
TRACE_KEY_INIT(UNTRACKED_STATS);
trace_printf_key(_untracked_stats,
diff --git a/dir.h b/dir.h
index bf23a470a..650e54bdf 100644
--- a/dir.h
+++ b/dir.h
@@ -151,7 +151,8 @@ struct dir_struct {
DIR_NO_GITLINKS = 1<<3,
DIR_COLLECT_IGNORED = 1<<4,
DIR_SHOW_IGNORED_TOO = 1<<5,
-   DIR_COLLECT_KILLED_ONLY = 1<<6
+   DIR_COLLECT_KILLED_ONLY = 1<<6,
+   DIR_KEEP_UNTRACKED_CONTENTS = 1<<7
} flags;
struct dir_entry **entries;
struct dir_entry **ignored;
diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 15e7592b6..fc6013ba3 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -12,7 +12,7 @@ cat >expected <<\EOF
 !! untracked/ignored
 EOF
 
-test_expect_failure 'status untracked directory with --ignored' '
+test_expect_success 'status untracked directory with --ignored' '
echo &quo

[PATCH v4 1/6] t7300: clean -d should skip dirs with ignored files

[Samuel Lijin]

If git sees a directory which contains only untracked and ignored
files, clean -d should not remove that directory. It was recently
discovered that this is *not* true of git clean -d, and it's possible
that this has never worked correctly; this test and its accompanying
patch series aims to fix that.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7300-clean.sh | 16 
 1 file changed, 16 insertions(+)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index b89fd2a6a..3a2d709c2 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,4 +653,20 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
+test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+   echo /foo/bar >.gitignore &&
+   echo ignoreme >>.gitignore &&
+   rm -rf foo &&
+   mkdir -p foo/a/aa/aaa foo/b/bb/bbb &&
+   touch foo/bar foo/baz foo/a/aa/ignoreme foo/b/ignoreme foo/b/bb/1 
foo/b/bb/2 &&
+   git clean -df &&
+   test_path_is_dir foo &&
+   test_path_is_file foo/bar &&
+   test_path_is_missing foo/baz &&
+   test_path_is_file foo/a/aa/ignoreme &&
+   test_path_is_missing foo/a/aa/aaa &&
+   test_path_is_file foo/b/ignoreme &&
+   test_path_is_missing foo/b/bb
+'
+
 test_done
-- 
2.13.0

[PATCH v4 5/6] dir: expose cmp_name() and check_contains()

[Samuel Lijin]

We want to use cmp_name() and check_contains() (which both compare
`struct dir_entry`s, the former in terms of the sort order, the latter
in terms of whether one lexically contains another) outside of dir.c,
so we have to (1) change their linkage and (2) rename them as
appropriate for the global namespace. The second is achieved by
renaming cmp_name() to cmp_dir_entry() and check_contains() to
check_dir_entry_contains().

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 11 ++-
 dir.h |  3 +++
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index 4b0a99323..2c520f17a 100644
--- a/dir.c
+++ b/dir.c
@@ -1842,7 +1842,7 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
return dir_state;
 }
 
-static int cmp_name(const void *p1, const void *p2)
+int cmp_dir_entry(const void *p1, const void *p2)
 {
const struct dir_entry *e1 = *(const struct dir_entry **)p1;
const struct dir_entry *e2 = *(const struct dir_entry **)p2;
@@ -1851,7 +1851,7 @@ static int cmp_name(const void *p1, const void *p2)
 }
 
 /* check if *out lexically strictly contains *in */
-static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+int check_dir_entry_contains(const struct dir_entry *out, const struct 
dir_entry *in)
 {
return (out->len < in->len) &&
(out->name[out->len - 1] == '/') &&
@@ -2071,8 +2071,8 @@ int read_directory(struct dir_struct *dir, const char 
*path,
dir->untracked = NULL;
if (!len || treat_leading_path(dir, path, len, pathspec))
read_directory_recursive(dir, path, len, untracked, 0, 
pathspec);
-   QSORT(dir->entries, dir->nr, cmp_name);
-   QSORT(dir->ignored, dir->ignored_nr, cmp_name);
+   QSORT(dir->entries, dir->nr, cmp_dir_entry);
+   QSORT(dir->ignored, dir->ignored_nr, cmp_dir_entry);
 
/* if DIR_SHOW_IGNORED_TOO, read_directory_recursive() will also pick
 * up untracked contents of untracked dirs; by default we discard these,
@@ -2084,7 +2084,8 @@ int read_directory(struct dir_struct *dir, const char 
*path,
 
/* remove from dir->entries untracked contents of untracked 
dirs */
for (i = j = 0; j < dir->nr; j++) {
-   if (i && check_contains(dir->entries[i - 1], 
dir->entries[j])) {
+   if (i &&
+   check_dir_entry_contains(dir->entries[i - 1], 
dir->entries[j])) {
free(dir->entries[j]);
dir->entries[j] = NULL;
} else {
diff --git a/dir.h b/dir.h
index 650e54bdf..edb5fda58 100644
--- a/dir.h
+++ b/dir.h
@@ -327,6 +327,9 @@ static inline int dir_path_match(const struct dir_entry 
*ent,
  has_trailing_dir);
 }
 
+int cmp_dir_entry(const void *p1, const void *p2);
+int check_dir_entry_contains(const struct dir_entry *out, const struct 
dir_entry *in);
+
 void untracked_cache_invalidate_path(struct index_state *, const char *);
 void untracked_cache_remove_from_index(struct index_state *, const char *);
 void untracked_cache_add_to_index(struct index_state *, const char *);
-- 
2.13.0

[PATCH v4 3/6] dir: recurse into untracked dirs for ignored files

[Samuel Lijin]

We consider directories containing only untracked and ignored files to
be themselves untracked, which in the usual case means we don't have to
search these directories. This is problematic when we want to collect
ignored files with DIR_SHOW_IGNORED_TOO, though, so we teach
read_directory_recursive() to recurse into untracked directories to find
the ignored files they contain when DIR_SHOW_IGNORED_TOO is set. This
has the side effect of also collecting all untracked files in untracked
directories as well.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index f451bfa48..68cf6e47c 100644
--- a/dir.c
+++ b/dir.c
@@ -1784,7 +1784,10 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
dir_state = state;
 
/* recurse into subdir if instructed by treat_path */
-   if (state == path_recurse) {
+   if ((state == path_recurse) ||
+   ((state == path_untracked) &&
+(dir->flags & DIR_SHOW_IGNORED_TOO) &&
+(get_dtype(cdir.de, path.buf, path.len) == DT_DIR))) {
struct untracked_cache_dir *ud;
ud = lookup_untracked(dir->untracked, untracked,
  path.buf + baselen,
-- 
2.13.0

[PATCH v4 2/6] t7061: status --ignored should search untracked dirs

[Samuel Lijin]

Per eb8c5b87, `status --ignored` by design does not list ignored files
if they are in a directory which contains only ignored and untracked
files (which is itself considered to be untracked) without `-uall`. This
does not make sense for `--ignored`, which claims to "Show ignored files
as well."

Thus we revisit eb8c5b87 and decide that for such directories, `status
--ignored` will list the directory as untracked *and* list all ignored
files within said directory even without `-uall`.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7061-wtstatus-ignore.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index cdc0747bf..15e7592b6 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -9,9 +9,10 @@ cat >expected <<\EOF
 ?? actual
 ?? expected
 ?? untracked/
+!! untracked/ignored
 EOF
 
-test_expect_success 'status untracked directory with --ignored' '
+test_expect_failure 'status untracked directory with --ignored' '
echo "ignored" >.gitignore &&
mkdir untracked &&
: >untracked/ignored &&
@@ -20,7 +21,7 @@ test_expect_success 'status untracked directory with 
--ignored' '
test_cmp expected actual
 '
 
-test_expect_success 'same with gitignore starting with BOM' '
+test_expect_failure 'same with gitignore starting with BOM' '
printf "\357\273\277ignored\n" >.gitignore &&
mkdir -p untracked &&
: >untracked/ignored &&
-- 
2.13.0

[PATCH v4 6/6] clean: teach clean -d to skip dirs containing ignored files

[Samuel Lijin]

There is an implicit assumption that a directory containing only
untracked and ignored files should itself be considered untracked. This
makes sense in use cases where we're asking if a directory should be
added to the git database, but not when we're asking if a directory can
be safely removed from the working tree; as a result, clean -d would
assume that an "untracked" directory containing ignored files could be
deleted.

To get around this, we teach clean -d to collect ignored files and skip
over so-called "untracked" directories if they contain any ignored
files (while still removing the untracked contents of such dirs).

This also fixes the known breakage in t7300 since clean -d now skips
untracked directories containing ignored files.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 builtin/clean.c  | 38 +-
 t/t7300-clean.sh |  2 +-
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index d861f836a..0d490d76e 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -859,7 +859,7 @@ static void interactive_main_loop(void)
 
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
-   int i, res;
+   int i, j, res;
int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
@@ -911,6 +911,9 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
  " refusing to clean"));
}
 
+   if (remove_directories)
+   dir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;
+
if (force > 1)
rm_flags = 0;
 
@@ -932,12 +935,39 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
 
fill_directory(, );
 
+   for (j = i = 0; i < dir.nr;) {
+   for (;
+j < dir.ignored_nr &&
+  0 <= cmp_dir_entry([i], [j]);
+j++);
+
+   if ((j < dir.ignored_nr) &&
+   check_dir_entry_contains(dir.entries[i], 
dir.ignored[j])) {
+   /* skip any dir.entries which contains a dir.ignored */
+   free(dir.entries[i]);
+   dir.entries[i++] = NULL;
+   } else {
+   /* prune the contents of a dir.entries which will be 
removed */
+   struct dir_entry *ent = dir.entries[i++];
+   for (;
+i < dir.nr &&
+  check_dir_entry_contains(ent, dir.entries[i]);
+i++) {
+   free(dir.entries[i]);
+   dir.entries[i] = NULL;
+   }
+   }
+   }
+
for (i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
int matches = 0;
struct stat st;
const char *rel;
 
+   if (!ent)
+   continue;
+
if (!cache_name_is_other(ent->name, ent->len))
continue;
 
@@ -958,6 +988,12 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
string_list_append(_list, rel);
}
 
+   for (i = 0; i < dir.nr; i++)
+   free(dir.entries[i]);
+
+   for (i = 0; i < dir.ignored_nr; i++)
+   free(dir.ignored[i]);
+
if (interactive && del_list.nr > 0)
interactive_main_loop();
 
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 3a2d709c2..7b36954d6 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,7 +653,7 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
-test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+test_expect_success 'git clean -d skips untracked dirs containing ignored 
files' '
echo /foo/bar >.gitignore &&
echo ignoreme >>.gitignore &&
rm -rf foo &&
-- 
2.13.0

Re: [PATCH v3 6/8] clean: teach clean -d to skip dirs containing ignored files

[Samuel Lijin]

On Wed, May 17, 2017 at 10:34 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Samuel Lijin <sxli...@gmail.com> writes:
>
>> @@ -932,7 +935,7 @@ int cmd_clean(int argc, const char **argv, const char 
>> *prefix)
>>
>>   fill_directory(, );
>>
>> - for (i = 0; i < dir.nr; i++) {
>> + for (k = i = 0; i < dir.nr; i++) {
>>   struct dir_entry *ent = dir.entries[i];
>>   int matches = 0;
>>   struct stat st;
>> @@ -954,10 +957,35 @@ int cmd_clean(int argc, const char **argv, const char 
>> *prefix)
>>   matches != MATCHED_EXACTLY)
>>   continue;
>>
>> + // skip any dir.entries which contains a dir.ignored
>> + for (; k < dir.ignored_nr; k++) {
>> + if (cmp_dir_entry([i],
>> + [k]) < 0)
>> + break;
>
> It is a bit unfortunate that we do not use the short-hand "ent" we
> established for dir.entries[i] at the beginning of this loop here.
>
>> + }
>> + if ((k < dir.ignored_nr) &&
>> + check_dir_entry_contains(dir.entries[i], 
>> dir.ignored[k])) {
>> + continue;
>> + }
>
> The above logic is not needed when dir.entries[i] is a directory,
> right?

Au contraire - this logic only matters when dir.entries[i] is a directory.

>> +
>> + // current entry does not contain any ignored files
>
> ... or the entry may not have been a directory in the first place.

If it's not a directory, it can't contain ignored files ;)

>>   rel = relative_path(ent->name, prefix, );
>>   string_list_append(_list, rel);
>> +
>> + // skip untracked contents of an untracked dir
>> + for (j = i + 1;
>> +  j < dir.nr &&
>> +  check_dir_entry_contains(dir.entries[i], 
>> dir.entries[j]);
>> +  j++);
>> + i = j - 1;
>>   }
>>
>> + for (i = 0; i < dir.nr; i++)
>> + free(dir.entries[i]);
>> +
>> + for (i = 0; i < dir.ignored_nr; i++)
>> + free(dir.ignored[i]);
>> +
>
> The above logic may not be incorrect per-se, but I have this
> suspicion that the resulting code may become cleaner and easier to
> understand if we did it in a new separate loop we call immediately
> after fill_directory() returns.  Or it could even be a call to a
> helper that "post processes" the "dir" thing that was polupated by
> fill_directory() that removes elements in the entries[] array that
> contains any element in ignored[] array.

Will try it out.

>>   if (interactive && del_list.nr > 0)
>>   interactive_main_loop();
>
> Thanks.

Re: Diff topic branch + working copy changes?

[Samuel Lijin]

On Wed, May 17, 2017 at 9:39 AM, Robert Dailey  wrote:
>
> Would be nice in the future to have another revision specification
> like @{wc} for "HEAD + working copy". I guess this technically isn't a
> revision, but something along those lines. Or maybe just an
> --include-wc for diff or something.

I think the spec would *just* be the working tree, if it ever got
implemented - I don't think it makes much sense to say "HEAD + working
copy".

Maybe something like this would do what you want?

# Diff Merge Base

dmb = "!f() { : git diff ; ref=$(git rev-parse --abbrev-ref HEAD); git
reset --soft $(git merge-base @{upstream} HEAD); git diff; git reset
--soft \"$ref\"; }; f"

Re: [PATCH v3 4/8] dir: hide untracked contents of untracked dirs

[Samuel Lijin]

On Wed, May 17, 2017 at 2:47 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Samuel Lijin <sxli...@gmail.com> writes:
>
>> When we taught read_directory_recursive() to recurse into untracked
>> directories in search of ignored files given DIR_SHOW_IGNORED_TOO, that
>> had the side effect of teaching it to collect the untracked contents of
>> untracked directories. It doesn't always make sense to return these,
>> though (we do need them for `clean -d`), so we introduce a flag
>> (DIR_KEEP_UNTRACKED_CONTENTS) to control whether or not read_directory()
>> strips dir->entries of the untracked contents of untracked dirs.
>>
>> We also introduce check_contains() to check if one dir_entry corresponds
>> to a path which contains the path corresponding to another dir_entry.
>>
>> Signed-off-by: Samuel Lijin <sxli...@gmail.com>
>> ---
>>  dir.c | 54 ++
>>  dir.h |  3 ++-
>>  2 files changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/dir.c b/dir.c
>> index 6bd0350e9..214a148ee 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1852,6 +1852,14 @@ static int cmp_name(const void *p1, const void *p2)
>>   return name_compare(e1->name, e1->len, e2->name, e2->len);
>>  }
>>
>> +/* check if *out lexically contains *in */
>> +static int check_contains(const struct dir_entry *out, const struct 
>> dir_entry *in)
>> +{
>> + return (out->len < in->len) &&
>> + (out->name[out->len - 1] == '/') &&
>> + !memcmp(out->name, in->name, out->len);
>> +}
>
> OK, treat_one_path() and treat_pah_fast() both ensure that a path to
> a directory is terminated with '/' before calling dir_add_name() and
> dir_add_ignored(), so we know a dir_entry "out" that is a directory
> must end with '/'.  Good.
>
> The second and third line being overly indented is a bit
> distracting, though.
>
>>  static int treat_leading_path(struct dir_struct *dir,
>> const char *path, int len,
>> const struct pathspec *pathspec)
>> @@ -2067,6 +2075,52 @@ int read_directory(struct dir_struct *dir, const char 
>> *path,
>>   read_directory_recursive(dir, path, len, untracked, 0, 
>> pathspec);
>>   QSORT(dir->entries, dir->nr, cmp_name);
>>   QSORT(dir->ignored, dir->ignored_nr, cmp_name);
>> +
>> + // if DIR_SHOW_IGNORED_TOO, read_directory_recursive() will also pick
>> + // up untracked contents of untracked dirs; by default we discard 
>> these,
>> + // but given DIR_KEEP_UNTRACKED_CONTENTS we do not
>
> /*
>  * Our multi-line comments are formatted like this
>  * example.  No C++/C99 // comments, outside of
>  * borrowed code and platform specific compat/ code,
>  * please.
>  */

Ga, I keep forgetting about this, sorry. (There has to be a way to
tell my compiler to catch this, right? It's pretty embarrassing to get
called out for this twice...)

>> + if ((dir->flags & DIR_SHOW_IGNORED_TOO)
>> +  && !(dir->flags & DIR_KEEP_UNTRACKED_CONTENTS)) {
>
> Both having && at the end and && at the beginning are valid C, but
> please stick to one style in a single file.

Got it.

>> + int i, j, nr_removed = 0;
>> +
>> + // remove from dir->entries untracked contents of untracked 
>> dirs
>
> /* And our single-liner comments look like this */
>
>> + for (i = 0; i < dir->nr; i++) {
>> + if (!dir->entries[i])
>> + continue;
>> +
>> + for (j = i + 1; j < dir->nr; j++) {
>> + if (!dir->entries[j])
>> + continue;
>> + if (check_contains(dir->entries[i], 
>> dir->entries[j])) {
>> + nr_removed++;
>> + free(dir->entries[j]);
>> + dir->entries[j] = NULL;
>> + }
>> + else {
>> + break;
>> + }
>> + }
>> + }
>
> This loop is O(n^2).  I wonder if we can do better, especially we
> know dir->entries[] is so

Re: [PATCH v3 3/8] dir: recurse into untracked dirs for ignored files

[Samuel Lijin]

On Wed, May 17, 2017 at 2:23 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Samuel Lijin <sxli...@gmail.com> writes:
>
>> We consider directories containing only untracked and ignored files to
>> be themselves untracked, which in the usual case means we don't have to
>> search these directories. This is problematic when we want to collect
>> ignored files with DIR_SHOW_IGNORED_TOO, though, so we teach
>> read_directory_recursive() to recurse into untracked directories to find
>> the ignored files they contain when DIR_SHOW_IGNORED_TOO is set. This
>> has the side effect of also collecting all untracked files in untracked
>> directories as well.
>>
>> Signed-off-by: Samuel Lijin <sxli...@gmail.com>
>> ---
>>  dir.c | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/dir.c b/dir.c
>> index f451bfa48..6bd0350e9 100644
>> --- a/dir.c
>> +++ b/dir.c
>> @@ -1784,7 +1784,12 @@ static enum path_treatment 
>> read_directory_recursive(struct dir_struct *dir,
>>   dir_state = state;
>>
>>   /* recurse into subdir if instructed by treat_path */
>> - if (state == path_recurse) {
>> + if ((state == path_recurse) ||
>> + ((get_dtype(cdir.de, path.buf, path.len) == 
>> DT_DIR) &&
>
> I see a conditional in treat_path() that is prepared to deal with a
> NULL in cdir.de; do we know cdir.de is always non-NULL at this point
> in the code, or is get_dtype() prepared to see NULL as its first
> parameter?
>
> ... goes and looks ...
>
> Yes, it falls back to the usual lstat() dance in such a case, so
> we'd be OK here.
>
>> +  (state == path_untracked) &&
>> +  (dir->flags & DIR_SHOW_IGNORED_TOO))
>
> If we are told to SHOW_IGNORED_TOO, we'd recurse into an untracked
> thing if it is a directory.  No other behaviour change.
>
> Isn't get_dtype() potentially slower than other two checks if it
> triggers?  I am wondering if these three conditions in &&-chain
> should be reordered to call get_dtype() the last, i.e.
>
> if ((state == path_recurse) ||
> ((dir->flags & DIR_SHOW_IGNORED_TOO)) &&
>  (state == path_untracked) &&
>  (get_dtype(cdir.de, path.buf, path.len) == DT_DIR)) {

Ah, I didn't realize that. I figured that get_dtype() was just a
helper to invoke the appropriate macros, but if it's actually mildly
expensive I'll take your reorder.

>> + )
>> + {
>
> It may be just a style, but these new lines are indented overly too
> deep.  We don't use a lone "{" on a line to open a block, either.
>
>>   struct untracked_cache_dir *ud;
>>   ud = lookup_untracked(dir->untracked, untracked,
>> path.buf + baselen,

Re: git-scm.com status report

[Samuel Lijin]

On Mon, Feb 6, 2017 at 1:27 PM, Jeff King  wrote:
> On Thu, Feb 02, 2017 at 03:33:50AM +0100, Jeff King wrote:
>
>> We (the Git project) got control of the git-scm.com domain this year. We
>> have never really had an "official" website, but I think a lot of people
>> consider this to be one.
>>
>> This is an overview of the current state, as well as some possible
>> issues and future work.
>
> Thanks everybody, for your responses here and off-list. After my mail
> got posted to HN, I got quite a lot of private responses, including
> offers to sponsor hosting, work on the site, etc. I'm still working my
> way through them, but I wanted to try to respond in aggregate here.
>
> First, a few clarifications:
>
>   - The money for the site wasn't mentioned to me by GitHub at all.  I'm
> quite sure they would continue to sponsor the site financially if
> need be. The only reason I didn't promise that is because I hadn't
> arranged it specifically, and "step 0" seemed like first making sure
> our costs were reasonable.
>
>   - Spinning the site out of GitHub's Heroku account isn't an urgent or
> impending change. It came out of a conversation I had with people
> auditing the GitHub account, where it is clearly a funny historical
> anomaly. So I suspect we could just stay there indefinitely if need
> be. But it seems to me like the right thing is to move it out for
> two reasons:
>
>   1. The site was always intended to serve the Git community, not
>  GitHub, and it has increasingly become a community asset (e.g.,
>  with the transfer of the domain name). The hosting assets
>  should be held by the community, too, to help with things like
>  continuity. If I get hit by a bus, the rest of the Git PLC
>  should have access to the site without having to figure out who
>  owns what.
>
>   2. Right now I can't add any other co-admins to handle operational
>  issues. So the bus factor and load of that part of operating
>  the site can't be spread.
>
> The responses I've gotten fall into a few buckets, I think:
>
>   - Yes, the current hosting cost really is unnecessarily high. Most of
> this is due to scaling wrong. The main costs are:
>
>   1. Using 2x dynos; these have 1GB of RAM versus 512MB. The site
>  does seem to use about 750MB. I have no idea why that is the
>  case. There's probably some low-hanging fruit in reducing the
>  memory use to keep it below 512MB, but I don't think anybody
>  has dug in there.
>
>   2. The site is scaled by using 3 dynos. It would be simpler and
>  cheaper to stick a CDN in front of it, since the pages change
>  very rarely. That's something I haven't looked into setting up
>  yet.
>
>  The prerequisite to using a CDN is actually making sure the
>  content is deterministic and cacheable. There was a nice PR
>  opened at https://github.com/git/git-scm.com/pull/941 towards
>  that end.
>
>   - It's mostly silly for this to be a Rails app at all. It's a static
> site which occasionally sucks in and formats new content (like the
> latest git version, new manpages, etc). The intent here was to make
> something that would "just run" forever and pick up new versions
> without human intervention. And that _does_ work, but it also makes
> things more expensive and complicated than they need to be.
>
> So a viable alternative is to use some kind of static site
> generator and have someone (or something) responsible for pulling in
> the new git versions occasionally.
>
> A few people have expressed interesting this. There's some
> preliminary work here:
>
>   https://github.com/git/git-scm.com/pull/941
>
> and at least GitLab has expressed some interest. So I'll let people
> coordinate in that PR or a new one what the result should look like.
> Working patches trump discussion. :)
>
> I have also talked with the GitHub Pages people, and they think
> hosting it as a Jekyll page wouldn't be a big deal performance-wise
> (with the caveat that we'd need to pre-render the asciidoctor bits
> ourselves, as Jekyll doesn't do asciidoc). So that's a viable option
> for hosting it for effectively free (though I think we _would_ still
> want to put a CDN in front of it). But if somebody has an
> alternative option, that's fine, too.
>
>   - Some people offered to help with running the site, or making major
> transitions (like converting to a static site). The most important
> thing to me there is that we have a solid maintenance plan. So I
> would want some evidence that anybody doing a major work would stick
> around in the community afterwards, or that it be done in a way that
> the handoff back to community members is easy. So I'd probably look
> for somebody already involved 

Re: [fixed] error using `git mergetool --tool=meld`

[Samuel Lijin]

On Tue, May 16, 2017 at 5:55 PM, Matthew Groth  wrote:
> I needed to use `git config mergetool.meld.hasOutput false`

Hm, that's interesting - the source code snippet you quoted from meld
implies it supports --output. What version of meld do you have?

David Aguilar seems to be the one who's contributed most of the meld
code, perhaps he might have an idea about why this is happening.

Re: bug with git add and .gitignore

[Samuel Lijin]

On Wed, May 10, 2017 at 9:44 AM, Orgad Shaneh  wrote:
> Hi,
>
> When a not-ignored file inside an ignore directory is added along with
> other files, a false alarm is shown:
>
> git init
> echo /d/ > .gitignore
> mkdir d
> touch d/file foo
> git add -f d/file foo
> git add d/file
> # fine
> git add d/file foo
> # The following paths are ignored by one of your .gitignore files:
> # d
> # Use -f if you really want to add them.

For anyone who wants to take a shot at this, the reason this happens
is because when "add d/file foo" is invoked, when add.c:cmd_add()
calls fill_directory(), d/ gets added to dir->ignored, so add_files()
complains. I'm not sure if that's *supposed* to happen, and the issue
is somewhere else, or the call to read_directory_recursive() from
fill_directory() doesn't properly respect the pathspec.

> I did not try to add a new file in d. It's the same file that is
> already indexed.
>
> - Orgad

Re: error using `git mergetool --tool=meld`

[Samuel Lijin]

Can you try `git config mergetool.meld.hasOutput true` and see if that
fixes the issue?

[PATCH v3 6/8] clean: teach clean -d to skip dirs containing ignored files

[Samuel Lijin]

There is an implicit assumption that a directory containing only
untracked and ignored files should itself be considered untracked. This
makes sense in use cases where we're asking if a directory should be
added to the git database, but not when we're asking if a directory can
be safely removed from the working tree; as a result, clean -d would
assume that an "untracked" directory containing ignored files could be
deleted.

To get around this, we teach clean -d to collect ignored files and skip
over so-called "untracked" directories if they contain any ignored
files (while still removing the untracked contents of such dirs).

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 builtin/clean.c | 32 ++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index d861f836a..25f3efce5 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -859,7 +859,7 @@ static void interactive_main_loop(void)
 
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
-   int i, res;
+   int i, j, k, res;
int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
@@ -911,6 +911,9 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
  " refusing to clean"));
}
 
+   if (remove_directories)
+   dir.flags |= DIR_SHOW_IGNORED_TOO | DIR_KEEP_UNTRACKED_CONTENTS;
+
if (force > 1)
rm_flags = 0;
 
@@ -932,7 +935,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
 
fill_directory(, );
 
-   for (i = 0; i < dir.nr; i++) {
+   for (k = i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
int matches = 0;
struct stat st;
@@ -954,10 +957,35 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
matches != MATCHED_EXACTLY)
continue;
 
+   // skip any dir.entries which contains a dir.ignored
+   for (; k < dir.ignored_nr; k++) {
+   if (cmp_dir_entry([i],
+   [k]) < 0)
+   break;
+   }
+   if ((k < dir.ignored_nr) &&
+   check_dir_entry_contains(dir.entries[i], 
dir.ignored[k])) {
+   continue;
+   }
+
+   // current entry does not contain any ignored files
rel = relative_path(ent->name, prefix, );
string_list_append(_list, rel);
+
+   // skip untracked contents of an untracked dir
+   for (j = i + 1;
+j < dir.nr &&
+check_dir_entry_contains(dir.entries[i], 
dir.entries[j]);
+j++);
+   i = j - 1;
}
 
+   for (i = 0; i < dir.nr; i++)
+   free(dir.entries[i]);
+
+   for (i = 0; i < dir.ignored_nr; i++)
+   free(dir.ignored[i]);
+
if (interactive && del_list.nr > 0)
interactive_main_loop();
 
-- 
2.12.2

[PATCH v3 8/8] t7061: status --ignored now searches untracked dirs

[Samuel Lijin]

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7061-wtstatus-ignore.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index 15e7592b6..fc6013ba3 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -12,7 +12,7 @@ cat >expected <<\EOF
 !! untracked/ignored
 EOF
 
-test_expect_failure 'status untracked directory with --ignored' '
+test_expect_success 'status untracked directory with --ignored' '
echo "ignored" >.gitignore &&
mkdir untracked &&
: >untracked/ignored &&
@@ -21,7 +21,7 @@ test_expect_failure 'status untracked directory with 
--ignored' '
test_cmp expected actual
 '
 
-test_expect_failure 'same with gitignore starting with BOM' '
+test_expect_success 'same with gitignore starting with BOM' '
printf "\357\273\277ignored\n" >.gitignore &&
mkdir -p untracked &&
: >untracked/ignored &&
-- 
2.12.2

[PATCH v3 4/8] dir: hide untracked contents of untracked dirs

[Samuel Lijin]

When we taught read_directory_recursive() to recurse into untracked
directories in search of ignored files given DIR_SHOW_IGNORED_TOO, that
had the side effect of teaching it to collect the untracked contents of
untracked directories. It doesn't always make sense to return these,
though (we do need them for `clean -d`), so we introduce a flag
(DIR_KEEP_UNTRACKED_CONTENTS) to control whether or not read_directory()
strips dir->entries of the untracked contents of untracked dirs.

We also introduce check_contains() to check if one dir_entry corresponds
to a path which contains the path corresponding to another dir_entry.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 54 ++
 dir.h |  3 ++-
 2 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index 6bd0350e9..214a148ee 100644
--- a/dir.c
+++ b/dir.c
@@ -1852,6 +1852,14 @@ static int cmp_name(const void *p1, const void *p2)
return name_compare(e1->name, e1->len, e2->name, e2->len);
 }
 
+/* check if *out lexically contains *in */
+static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+{
+   return (out->len < in->len) &&
+   (out->name[out->len - 1] == '/') &&
+   !memcmp(out->name, in->name, out->len);
+}
+
 static int treat_leading_path(struct dir_struct *dir,
  const char *path, int len,
  const struct pathspec *pathspec)
@@ -2067,6 +2075,52 @@ int read_directory(struct dir_struct *dir, const char 
*path,
read_directory_recursive(dir, path, len, untracked, 0, 
pathspec);
QSORT(dir->entries, dir->nr, cmp_name);
QSORT(dir->ignored, dir->ignored_nr, cmp_name);
+
+   // if DIR_SHOW_IGNORED_TOO, read_directory_recursive() will also pick
+   // up untracked contents of untracked dirs; by default we discard these,
+   // but given DIR_KEEP_UNTRACKED_CONTENTS we do not
+   if ((dir->flags & DIR_SHOW_IGNORED_TOO)
+&& !(dir->flags & DIR_KEEP_UNTRACKED_CONTENTS)) {
+   int i, j, nr_removed = 0;
+
+   // remove from dir->entries untracked contents of untracked dirs
+   for (i = 0; i < dir->nr; i++) {
+   if (!dir->entries[i])
+   continue;
+
+   for (j = i + 1; j < dir->nr; j++) {
+   if (!dir->entries[j])
+   continue;
+   if (check_contains(dir->entries[i], 
dir->entries[j])) {
+   nr_removed++;
+   free(dir->entries[j]);
+   dir->entries[j] = NULL;
+   }
+   else {
+   break;
+   }
+   }
+   }
+
+   // strip dir->entries of NULLs
+   if (nr_removed) {
+   for (i = 0;;) {
+   while (i < dir->nr && dir->entries[i])
+   i++;
+   if (i == dir->nr)
+   break;
+   j = i;
+   while (j < dir->nr && !dir->entries[j])
+   j++;
+   if (j == dir->nr)
+   break;
+   dir->entries[i] = dir->entries[j];
+   dir->entries[j] = NULL;
+   }
+   dir->nr -= nr_removed;
+   }
+   }
+
if (dir->untracked) {
static struct trace_key trace_untracked_stats = 
TRACE_KEY_INIT(UNTRACKED_STATS);
trace_printf_key(_untracked_stats,
diff --git a/dir.h b/dir.h
index bf23a470a..650e54bdf 100644
--- a/dir.h
+++ b/dir.h
@@ -151,7 +151,8 @@ struct dir_struct {
DIR_NO_GITLINKS = 1<<3,
DIR_COLLECT_IGNORED = 1<<4,
DIR_SHOW_IGNORED_TOO = 1<<5,
-   DIR_COLLECT_KILLED_ONLY = 1<<6
+   DIR_COLLECT_KILLED_ONLY = 1<<6,
+   DIR_KEEP_UNTRACKED_CONTENTS = 1<<7
} flags;
struct dir_entry **entries;
struct dir_entry **ignored;
-- 
2.12.2

[PATCH v3 3/8] dir: recurse into untracked dirs for ignored files

[Samuel Lijin]

We consider directories containing only untracked and ignored files to
be themselves untracked, which in the usual case means we don't have to
search these directories. This is problematic when we want to collect
ignored files with DIR_SHOW_IGNORED_TOO, though, so we teach
read_directory_recursive() to recurse into untracked directories to find
the ignored files they contain when DIR_SHOW_IGNORED_TOO is set. This
has the side effect of also collecting all untracked files in untracked
directories as well.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index f451bfa48..6bd0350e9 100644
--- a/dir.c
+++ b/dir.c
@@ -1784,7 +1784,12 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
dir_state = state;
 
/* recurse into subdir if instructed by treat_path */
-   if (state == path_recurse) {
+   if ((state == path_recurse) ||
+   ((get_dtype(cdir.de, path.buf, path.len) == 
DT_DIR) &&
+(state == path_untracked) &&
+(dir->flags & DIR_SHOW_IGNORED_TOO))
+   )
+   {
struct untracked_cache_dir *ud;
ud = lookup_untracked(dir->untracked, untracked,
  path.buf + baselen,
-- 
2.12.2

[PATCH v3 5/8] dir: expose cmp_name() and check_contains()

[Samuel Lijin]

We want to use cmp_name() and check_contains() (which both compare
`struct dir_entry`s, the former in terms of the sort order, the latter
in terms of whether one lexically contains another) outside of dir.c,
so we have to (1) change their linkage and (2) rename them as
appropriate for the global namespace. The second is achieved by
renaming cmp_name() to cmp_dir_entry() and check_contains() to
check_dir_entry_contains().

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 10 +-
 dir.h |  3 +++
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index 214a148ee..1bf2d20e7 100644
--- a/dir.c
+++ b/dir.c
@@ -1844,7 +1844,7 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
return dir_state;
 }
 
-static int cmp_name(const void *p1, const void *p2)
+int cmp_dir_entry(const void *p1, const void *p2)
 {
const struct dir_entry *e1 = *(const struct dir_entry **)p1;
const struct dir_entry *e2 = *(const struct dir_entry **)p2;
@@ -1853,7 +1853,7 @@ static int cmp_name(const void *p1, const void *p2)
 }
 
 /* check if *out lexically contains *in */
-static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+int check_dir_entry_contains(const struct dir_entry *out, const struct 
dir_entry *in)
 {
return (out->len < in->len) &&
(out->name[out->len - 1] == '/') &&
@@ -2073,8 +2073,8 @@ int read_directory(struct dir_struct *dir, const char 
*path,
dir->untracked = NULL;
if (!len || treat_leading_path(dir, path, len, pathspec))
read_directory_recursive(dir, path, len, untracked, 0, 
pathspec);
-   QSORT(dir->entries, dir->nr, cmp_name);
-   QSORT(dir->ignored, dir->ignored_nr, cmp_name);
+   QSORT(dir->entries, dir->nr, cmp_dir_entry);
+   QSORT(dir->ignored, dir->ignored_nr, cmp_dir_entry);
 
// if DIR_SHOW_IGNORED_TOO, read_directory_recursive() will also pick
// up untracked contents of untracked dirs; by default we discard these,
@@ -2091,7 +2091,7 @@ int read_directory(struct dir_struct *dir, const char 
*path,
for (j = i + 1; j < dir->nr; j++) {
if (!dir->entries[j])
continue;
-   if (check_contains(dir->entries[i], 
dir->entries[j])) {
+   if (check_dir_entry_contains(dir->entries[i], 
dir->entries[j])) {
nr_removed++;
free(dir->entries[j]);
dir->entries[j] = NULL;
diff --git a/dir.h b/dir.h
index 650e54bdf..edb5fda58 100644
--- a/dir.h
+++ b/dir.h
@@ -327,6 +327,9 @@ static inline int dir_path_match(const struct dir_entry 
*ent,
  has_trailing_dir);
 }
 
+int cmp_dir_entry(const void *p1, const void *p2);
+int check_dir_entry_contains(const struct dir_entry *out, const struct 
dir_entry *in);
+
 void untracked_cache_invalidate_path(struct index_state *, const char *);
 void untracked_cache_remove_from_index(struct index_state *, const char *);
 void untracked_cache_add_to_index(struct index_state *, const char *);
-- 
2.12.2

[PATCH v3 2/8] t7061: status --ignored should search untracked dirs

[Samuel Lijin]

Per eb8c5b87, `status --ignored` by design does not list ignored files
if they are in a directory which contains only ignored and untracked
files (which is itself considered to be untracked) without `-uall`. This
does not make sense for `--ignored`, which claims to "Show ignored files
as well."

Thus we revisit eb8c5b87 and decide that for such directories, `status
--ignored` will list the directory as untracked *and* list all ignored
files within said directory even without `-uall`.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7061-wtstatus-ignore.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index cdc0747bf..15e7592b6 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -9,9 +9,10 @@ cat >expected <<\EOF
 ?? actual
 ?? expected
 ?? untracked/
+!! untracked/ignored
 EOF
 
-test_expect_success 'status untracked directory with --ignored' '
+test_expect_failure 'status untracked directory with --ignored' '
echo "ignored" >.gitignore &&
mkdir untracked &&
: >untracked/ignored &&
@@ -20,7 +21,7 @@ test_expect_success 'status untracked directory with 
--ignored' '
test_cmp expected actual
 '
 
-test_expect_success 'same with gitignore starting with BOM' '
+test_expect_failure 'same with gitignore starting with BOM' '
printf "\357\273\277ignored\n" >.gitignore &&
mkdir -p untracked &&
: >untracked/ignored &&
-- 
2.12.2

[PATCH v3 7/8] t7300: clean -d now skips untracked dirs containing ignored files

[Samuel Lijin]

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7300-clean.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index c14c98e56..b65d4719c 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,7 +653,7 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
-test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+test_expect_success 'git clean -d skips untracked dirs containing ignored 
files' '
echo /foo/bar >.gitignore &&
rm -rf foo &&
mkdir foo &&
-- 
2.12.2

[PATCH v3 1/8] t7300: clean -d should skip dirs with ignored files

[Samuel Lijin]

If git sees a directory which contains only untracked and ignored
files, clean -d should not remove that directory. It was recently
discovered that this is *not* true of git clean -d, and it's possible
that this has never worked correctly; this test and its accompanying
patch series aims to fix that.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7300-clean.sh | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index b89fd2a6a..c14c98e56 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,4 +653,15 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
+test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+   echo /foo/bar >.gitignore &&
+   rm -rf foo &&
+   mkdir foo &&
+   touch foo/bar foo/baz &&
+   git clean -df &&
+   test_path_is_dir foo &&
+   test_path_is_file foo/bar &&
+   test_path_is_missing foo/baz
+'
+
 test_done
-- 
2.12.2

[PATCH v3 0/8] Fix clean -d and status --ignored

[Samuel Lijin]

Restructures the patch series so that everything works properly.

I also noticed while redoing it that I didn't properly fix the clean -d bug
before; while it wouldn't remove a directory with only untracked and ignored
files, it would miss the untracked contents of such a directory. That's now been
fixed.

Samuel Lijin (8):
  t7300: clean -d should skip dirs with ignored files
  t7061: status --ignored should search untracked dirs
  dir: recurse into untracked dirs for ignored files
  dir: hide untracked contents of untracked dirs
  dir: expose cmp_name() and check_contains()
  clean: teach clean -d to skip dirs containing ignored files
  t7300: clean -d now skips untracked dirs containing ignored files
  t7061: status --ignored now searches untracked dirs

 builtin/clean.c| 32 --
 dir.c  | 67 +++---
 dir.h  |  6 -
 t/t7061-wtstatus-ignore.sh |  1 +
 t/t7300-clean.sh   | 11 
 5 files changed, 110 insertions(+), 7 deletions(-)

-- 
2.12.2

Re: git log --follow after subtree merge

[Samuel Lijin]

On Wed, May 10, 2017 at 9:46 AM, Jonny Gilchrist
 wrote:
> Hi,
>
> After doing a subtree merge, using 'git log' and 'git log --follow' on
> files in the subtree show only the merge commit in which they were
> added.
>
> After reading around I understand that the issue is that git log
> --follow doesn't track renames that occur during a merge.

Try git log --follow -M. (You may also want to combine this with -l and/or -C).

> Has there been any work (or are there any plans) to allow git log
> --follow to work in this case? I couldn't find anything in the mailing
> list archives aside from a couple of threads from 2011 explaining the
> issue.
>
> Thanks,
> J.

Re: [PATCH v2 0/9] Keep git clean -d from inadvertently removing ignored files

[Samuel Lijin]

On Sun, May 7, 2017 at 11:26 PM, Junio C Hamano <gits...@pobox.com> wrote:
> Samuel Lijin <sxli...@gmail.com> writes:
>
>> Addresses the issues raised by Stefan and Junio (thanks for your
>> feedback) about not using C99-style comments and keeping tests
>> working on every commit to prevent breaking git bisect. (About the
>> latter one: is it necessary to prevent compiler warnings, in
>> addition to compiler errors? Because if so I should probably
>> squash some of the commits together.)
>
> Some of us build with -Werror, so yes.  If by "squashing" you mean
> "instead of piling a fix on top of a broken patch, I need to do
> things right from the beginning", then yes, please do so, not just
> for compiler warnings but for all forms of changes.

Got it - will keep this in mind when I reroll the patch series.

>> Note that this introduces a breaking change in the behavior of git
>> status: when invoked with --ignored, git status will now return
>> ignored files in an untracked directory, whereas previously it
>> would not.
>
> What do you mean by a "breaking change"?  Is it just "a new bug"?
> Or "the current behaviour is logically broken, but people and
> scripts might have relied on that odd behaviour and fixing it this
> late in the game would break their expectations"?

The latter, as I believe you noticed in your reply about patch 9/9.

What happens right now is that because (1) directories containing only
untracked and ignored files are considered "untracked" and (2)
read_directory_recursive() skips over untracked directories, even with
DIR_SHOW_IGNORED_TOO set. As a result, `status --ignored` never lists
ignored files if they're in an "untracked" directory (and this is the
currently defined behavior per t7061).

By teaching read_directory_recursive() to recurse into untracked
directories in search of ignored files when DIR_SHOW_IGNORED_TOO is
set, though, `status --ignored` now learns to report the existence of
these ignored files, whereas previously it did not.

>> It's possible that there are standard practices that I might have
>> missed, so if there is anything along those lines, I'd appreciate
>> you letting me know. (As an aside, about the git bisect thing: is
>> there a script somewhere that people use to test patch series
>> before sending them out?)
>
> I hear that people use variations of
>
> git rebase -x "make test"
>
> on their topic.

Aha - thanks.

Re: [PATCH v2 1/9] t7300: skip untracked dirs containing ignored files

[Samuel Lijin]

On Sun, May 7, 2017 at 2:12 PM, Torsten Bögershausen <tbo...@web.de> wrote:
> On 2017-05-05 12:46, Samuel Lijin wrote:
>> If git sees a directory which contains only untracked and ignored
>> files, clean -d should not remove that directory. It was recently
>> discovered that this is *not* true of git clean -d, and it's possible
>> that this has never worked correctly; this test and its accompanying
>> patch series aims to fix that.
>>
>> Signed-off-by: Samuel Lijin <sxli...@gmail.com>
>> ---
>>  t/t7300-clean.sh | 10 ++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
>> index b89fd2a6a..252c75b40 100755
>> --- a/t/t7300-clean.sh
>> +++ b/t/t7300-clean.sh
>> @@ -653,4 +653,14 @@ test_expect_success 'git clean -d respects pathspecs 
>> (pathspec is prefix of dir)
>>   test_path_is_dir foobar
>>  '
>>
>> +test_expect_failure 'git clean -d skips untracked dirs containing ignored 
>> files' '
>> + echo /foo/bar >.gitignore &&
>> + rm -rf foo &&
>> + mkdir -p foo &&
> Minor remark:
> Do we need the -p here, or can it be dropped?

Yeah, the -p isn't needed here - will change when I reroll.

>> + touch foo/bar &&
>> + git clean -df &&
>> + test_path_is_file foo/bar &&
>> + test_path_is_dir foo
>> +'
>> +
>>  test_done
>>
>

Re: vger not relaying some of Junio's messages today?

[Samuel Lijin]

On Sat, May 6, 2017 at 3:50 PM, Eric Wong <e...@80x24.org> wrote:
> (I have no idea what Jonathan Tirado wrote; it was encrypted (but
>  sent to a public list).
>
> Samuel Lijin <sxli...@gmail.com> wrote:
>> Yep, I see these on public-inbox.org/git/ but not in my gmail inbox:
>
> Hi Samuel, check your Spam box (and move it to a normal inbox so
> they can train it).  Gmail filters are known to trigger happy
> and incorrectly flag messages.  It's been a problem on LKML,
> too.

Sorry, should've been clearer - I did check my spambox in my original
message. Some old patches from Brandon were in there, but the ones I
mentioned in my original message just seem to have been dropped.

[PATCH v2 5/9] dir: hide untracked contents of untracked dirs

[Samuel Lijin]

When we taught read_directory_recursive() to recurse into untracked
directories in search of ignored files given DIR_SHOW_IGNORED_TOO, that
had the side effect of teaching it to collect the untracked contents of
untracked directories. It does not make sense to return these, so we
teach read_directory() to strip dir->entries of any such untracked
contents.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 44 
 1 file changed, 44 insertions(+)

diff --git a/dir.c b/dir.c
index 4739087f4..fd445ee9e 100644
--- a/dir.c
+++ b/dir.c
@@ -2075,6 +2075,50 @@ int read_directory(struct dir_struct *dir, const char 
*path,
read_directory_recursive(dir, path, len, untracked, 0, 
pathspec);
QSORT(dir->entries, dir->nr, cmp_name);
QSORT(dir->ignored, dir->ignored_nr, cmp_name);
+
+   // if collecting ignored files, never consider a directory containing
+   // ignored files to be untracked
+   if (dir->flags & DIR_SHOW_IGNORED_TOO) {
+   int i, j, nr_removed = 0;
+
+   // remove from dir->entries untracked contents of untracked dirs
+   for (i = 0; i < dir->nr; i++) {
+   if (!dir->entries[i])
+   continue;
+
+   for (j = i + 1; j < dir->nr; j++) {
+   if (!dir->entries[j])
+   continue;
+   if (check_contains(dir->entries[i], 
dir->entries[j])) {
+   nr_removed++;
+   free(dir->entries[j]);
+   dir->entries[j] = NULL;
+   }
+   else {
+   break;
+   }
+   }
+   }
+
+   // strip dir->entries of NULLs
+   if (nr_removed) {
+   for (i = 0;;) {
+   while (i < dir->nr && dir->entries[i])
+   i++;
+   if (i == dir->nr)
+   break;
+   j = i;
+   while (j < dir->nr && !dir->entries[j])
+   j++;
+   if (j == dir->nr)
+   break;
+   dir->entries[i] = dir->entries[j];
+   dir->entries[j] = NULL;
+   }
+   dir->nr -= nr_removed;
+   }
+   }
+
if (dir->untracked) {
static struct trace_key trace_untracked_stats = 
TRACE_KEY_INIT(UNTRACKED_STATS);
trace_printf_key(_untracked_stats,
-- 
2.12.2

[PATCH v2 8/9] t7300: clean -d now skips untracked dirs containing ignored files

[Samuel Lijin]

This was previously broken (and likely never worked); this concludes the
patch series fixing the behavior of clean -d.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7300-clean.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 252c75b40..948a455e8 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,7 +653,7 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
-test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+test_expect_success 'git clean -d skips untracked dirs containing ignored 
files' '
echo /foo/bar >.gitignore &&
rm -rf foo &&
mkdir -p foo &&
-- 
2.12.2

[PATCH v2 9/9] t7061: expect ignored files in untracked dirs

[Samuel Lijin]

We now expect `status --ignored` to list ignored files even if they are
in an untracked directory.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7061-wtstatus-ignore.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index dc3be92a2..fc6013ba3 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -9,9 +9,10 @@ cat >expected <<\EOF
 ?? actual
 ?? expected
 ?? untracked/
+!! untracked/ignored
 EOF
 
-test_expect_failure 'status untracked directory with --ignored' '
+test_expect_success 'status untracked directory with --ignored' '
echo "ignored" >.gitignore &&
mkdir untracked &&
: >untracked/ignored &&
@@ -20,7 +21,7 @@ test_expect_failure 'status untracked directory with 
--ignored' '
test_cmp expected actual
 '
 
-test_expect_failure 'same with gitignore starting with BOM' '
+test_expect_success 'same with gitignore starting with BOM' '
printf "\357\273\277ignored\n" >.gitignore &&
mkdir -p untracked &&
: >untracked/ignored &&
-- 
2.12.2

[PATCH v2 7/9] builtin/clean: teach clean -d to skip dirs containing ignored files

[Samuel Lijin]

There is an implicit assumption that a directory containing only
untracked and ignored files should itself be considered untracked. This
makes sense in use cases where we're asking if a directory should be
added to the git database, but not when we're asking if a directory can
be safely removed from the working tree; as a result, clean -d would
assume that an "untracked" directory containing ignored files could be
deleted.

To get around this, we teach clean -d to collect ignored files and skip
over so-called "untracked" directories if they contain any ignored
files.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 builtin/clean.c | 24 ++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index d861f836a..368e19427 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -859,7 +859,7 @@ static void interactive_main_loop(void)
 
 int cmd_clean(int argc, const char **argv, const char *prefix)
 {
-   int i, res;
+   int i, j, res;
int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
@@ -911,6 +911,9 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
  " refusing to clean"));
}
 
+   if (remove_directories)
+   dir.flags |= DIR_SHOW_IGNORED_TOO;
+
if (force > 1)
rm_flags = 0;
 
@@ -932,7 +935,7 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
 
fill_directory(, );
 
-   for (i = 0; i < dir.nr; i++) {
+   for (j = i = 0; i < dir.nr; i++) {
struct dir_entry *ent = dir.entries[i];
int matches = 0;
struct stat st;
@@ -954,10 +957,27 @@ int cmd_clean(int argc, const char **argv, const char 
*prefix)
matches != MATCHED_EXACTLY)
continue;
 
+   // skip any dir.entries which contains a dir.ignored
+   for (; j < dir.ignored_nr; j++) {
+   if (cmp_name([i],
+   [j]) < 0)
+   break;
+   }
+   if ((j < dir.ignored_nr) &&
+   check_contains(dir.entries[i], dir.ignored[j])) 
{
+   continue;
+   }
+
rel = relative_path(ent->name, prefix, );
string_list_append(_list, rel);
}
 
+   for (i = 0; i < dir.nr; i++)
+   free(dir.entries[i]);
+
+   for (i = 0; i < dir.ignored_nr; i++)
+   free(dir.ignored[i]);
+
if (interactive && del_list.nr > 0)
interactive_main_loop();
 
-- 
2.12.2

[PATCH v2 6/9] dir: change linkage of cmp_name() and check_contains()

[Samuel Lijin]

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 4 ++--
 dir.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index fd445ee9e..6a71683df 100644
--- a/dir.c
+++ b/dir.c
@@ -1844,7 +1844,7 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
return dir_state;
 }
 
-static int cmp_name(const void *p1, const void *p2)
+int cmp_name(const void *p1, const void *p2)
 {
const struct dir_entry *e1 = *(const struct dir_entry **)p1;
const struct dir_entry *e2 = *(const struct dir_entry **)p2;
@@ -1853,7 +1853,7 @@ static int cmp_name(const void *p1, const void *p2)
 }
 
 /* check if *out lexically contains *in */
-static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+int check_contains(const struct dir_entry *out, const struct dir_entry *in)
 {
return (out->len < in->len) &&
(out->name[out->len - 1] == '/') &&
diff --git a/dir.h b/dir.h
index bf23a470a..1ddd8b611 100644
--- a/dir.h
+++ b/dir.h
@@ -326,6 +326,9 @@ static inline int dir_path_match(const struct dir_entry 
*ent,
  has_trailing_dir);
 }
 
+int cmp_name(const void *p1, const void *p2);
+int check_contains(const struct dir_entry *out, const struct dir_entry *in);
+
 void untracked_cache_invalidate_path(struct index_state *, const char *);
 void untracked_cache_remove_from_index(struct index_state *, const char *);
 void untracked_cache_add_to_index(struct index_state *, const char *);
-- 
2.12.2

[PATCH v2 4/9] dir: add method to check if a dir_entry lexically contains another

[Samuel Lijin]

Introduce a method that allows us to check if one dir_entry corresponds
to a path which contains the path corresponding to another dir_entry.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/dir.c b/dir.c
index 6bd0350e9..4739087f4 100644
--- a/dir.c
+++ b/dir.c
@@ -1852,6 +1852,14 @@ static int cmp_name(const void *p1, const void *p2)
return name_compare(e1->name, e1->len, e2->name, e2->len);
 }
 
+/* check if *out lexically contains *in */
+static int check_contains(const struct dir_entry *out, const struct dir_entry 
*in)
+{
+   return (out->len < in->len) &&
+   (out->name[out->len - 1] == '/') &&
+   !memcmp(out->name, in->name, out->len);
+}
+
 static int treat_leading_path(struct dir_struct *dir,
  const char *path, int len,
  const struct pathspec *pathspec)
-- 
2.12.2

[PATCH v2 3/9] dir: recurse into untracked dirs for ignored files

[Samuel Lijin]

We consider directories containing only untracked and ignored files to
be themselves untracked, which in the usual case means we don't have to
search these directories. This is problematic when we want to collect
ignored files with DIR_SHOW_IGNORED_TOO, though, so we teach
read_directory_recursive() to recurse into untracked directories to find
the ignored files they contain when DIR_SHOW_IGNORED_TOO is set. This
has the side effect of also collecting all untracked files in untracked
directories as well.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 dir.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/dir.c b/dir.c
index f451bfa48..6bd0350e9 100644
--- a/dir.c
+++ b/dir.c
@@ -1784,7 +1784,12 @@ static enum path_treatment 
read_directory_recursive(struct dir_struct *dir,
dir_state = state;
 
/* recurse into subdir if instructed by treat_path */
-   if (state == path_recurse) {
+   if ((state == path_recurse) ||
+   ((get_dtype(cdir.de, path.buf, path.len) == 
DT_DIR) &&
+(state == path_untracked) &&
+(dir->flags & DIR_SHOW_IGNORED_TOO))
+   )
+   {
struct untracked_cache_dir *ud;
ud = lookup_untracked(dir->untracked, untracked,
  path.buf + baselen,
-- 
2.12.2

[PATCH v2 2/9] t7061: expect failure where expected behavior will change

[Samuel Lijin]

This changes tests for `status --ignored` from test_expect_success to
test_expect_failure in preparation for a change in its expected behavior
(namely, that ignored files in untracked dirs will be reported).

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7061-wtstatus-ignore.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t7061-wtstatus-ignore.sh b/t/t7061-wtstatus-ignore.sh
index cdc0747bf..dc3be92a2 100755
--- a/t/t7061-wtstatus-ignore.sh
+++ b/t/t7061-wtstatus-ignore.sh
@@ -11,7 +11,7 @@ cat >expected <<\EOF
 ?? untracked/
 EOF
 
-test_expect_success 'status untracked directory with --ignored' '
+test_expect_failure 'status untracked directory with --ignored' '
echo "ignored" >.gitignore &&
mkdir untracked &&
: >untracked/ignored &&
@@ -20,7 +20,7 @@ test_expect_success 'status untracked directory with 
--ignored' '
test_cmp expected actual
 '
 
-test_expect_success 'same with gitignore starting with BOM' '
+test_expect_failure 'same with gitignore starting with BOM' '
printf "\357\273\277ignored\n" >.gitignore &&
mkdir -p untracked &&
: >untracked/ignored &&
-- 
2.12.2

[PATCH v2 1/9] t7300: skip untracked dirs containing ignored files

[Samuel Lijin]

If git sees a directory which contains only untracked and ignored
files, clean -d should not remove that directory. It was recently
discovered that this is *not* true of git clean -d, and it's possible
that this has never worked correctly; this test and its accompanying
patch series aims to fix that.

Signed-off-by: Samuel Lijin <sxli...@gmail.com>
---
 t/t7300-clean.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index b89fd2a6a..252c75b40 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -653,4 +653,14 @@ test_expect_success 'git clean -d respects pathspecs 
(pathspec is prefix of dir)
test_path_is_dir foobar
 '
 
+test_expect_failure 'git clean -d skips untracked dirs containing ignored 
files' '
+   echo /foo/bar >.gitignore &&
+   rm -rf foo &&
+   mkdir -p foo &&
+   touch foo/bar &&
+   git clean -df &&
+   test_path_is_file foo/bar &&
+   test_path_is_dir foo
+'
+
 test_done
-- 
2.12.2

[PATCH v2 0/9] Keep git clean -d from inadvertently removing ignored files

[Samuel Lijin]

Addresses the issues raised by Stefan and Junio (thanks for your feedback) 
about not using C99-style comments and keeping tests working on every commit to 
prevent breaking git bisect. (About the latter one: is it necessary to prevent 
compiler warnings, in addition to compiler errors? Because if so I should 
probably squash some of the commits together.)

Note that this introduces a breaking change in the behavior of git status: when 
invoked with --ignored, git status will now return ignored files in an 
untracked directory, whereas previously it would not.

It's possible that there are standard practices that I might have missed, so if 
there is anything along those lines, I'd appreciate you letting me know. (As an 
aside, about the git bisect thing: is there a script somewhere that people use 
to test patch series before sending them out?)

Samuel Lijin (9):
  t7300: skip untracked dirs containing ignored files
  t7061: expect failure where expected behavior will change
  dir: recurse into untracked dirs for ignored files
  dir: add method to check if a dir_entry lexically contains another
  dir: hide untracked contents of untracked dirs
  dir: change linkage of cmp_name() and check_contains()
  builtin/clean: teach clean -d to skip dirs containing ignored files
  t7300: clean -d now skips untracked dirs containing ignored files
  t7061: expect ignored files in untracked dirs

 builtin/clean.c| 24 --
 dir.c  | 61 --
 dir.h  |  3 +++
 t/t7061-wtstatus-ignore.sh |  1 +
 t/t7300-clean.sh   | 10 
 5 files changed, 95 insertions(+), 4 deletions(-)

-- 
2.12.2

Re: vger not relaying some of Junio's messages today?

[Samuel Lijin]

Yep, I see these on public-inbox.org/git/ but not in my gmail inbox:

- Brandon [RFC 01/14] through [RFC 14/14] convert dir.c to take an
index parameter
- Johanne's Coverity patch series