Re: [PATCH v3 3/3] commit: fix exit code for --short/--porcelain
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=0x55a29960 ) at
wt-status.c:766
warning: Source file is more recent than executable.
760 for (i = 0; i < s->change.nr; i++) {
(gdb) print s->change.nr
$1 = 1
Can you confirm I'm not crazy, and am analyzing this correctly?
> I _think_ with the change to "what happens during merge" above that
> I suggested, this loop automatically becomes correct, but I didn't
> think it through. If there are ways other than .merge_in_progress
> that place conflicted entries in the index, then this loop is still
> incorrect and would want to be more like:
>
> for (i = 0; i < s->change.nr; i++) {
> struct
Re: [PATCH v3 3/3] commit: fix exit code for --short/--porcelain
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 _think_ with the change to "what happens during merge" above that
I suggested, this loop automatically becomes correct, but I didn't
think it through. If there are ways other than .merge_in_progress
that place conflicted entries in the index, then this loop is still
incorrect and would want to be more like:
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) {
s->committable = 0;
return;
}
if (d->index_status)
s->committable = 1;
}
i.e. we declare "not ready to commit" if there is *any* conflicted
entry, but otherwise set committable to 1 if we see any entry that
is different from HEAD (to declare succcess once we successfully
loop through to the last entry without seeing any conflict).
> 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;
> + }
No need to have {} around a single statement. Especially when you
know you won't be touching the line (e.g. to later add more
statements in the block) in this last patch in a series.
> + wt_longstatus_print_cached_header(s);
> +
[PATCH v3 3/3] commit: fix exit code for --short/--porcelain
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_UPDATED, it);
}
- if (shown_header)
- wt_longstatus_print_trailer(s);
+
+ wt_longstatus_print_trailer(s);
}
