[PATCH 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c

[PATCH 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c

[Johannes Schindelin]

This is the 5th last patch series of my work to accelerate interactive
rebases in particular on Windows.

Basically, all it does is to make reusable some functions that were
ported over from git-pull.sh but made private to builtin/pull.c.

An earlier attempt included only the first patch, and somehow it failed
to convince our good Git maintainer without mentioning that it is part
of something much bigger:

http://public-inbox.org/git/974d0bfed38e8aa410e97e05022bc5dbbd78d915.1457615785.git.johannes.schindelin@gmx.de/

However, now that I have this big carrot (3x speedup on Linux, 4x
speedup on MacOSX and 5x speedup on Windows), it cannot possibly fail.
*thumbs-crossed*


Johannes Schindelin (6):
  pull: drop confusing prefix parameter of die_on_unclean_work_tree()
  pull: make code more similar to the shell script again
  Make the require_clean_work_tree() function truly reusable
  require_clean_work_tree: ensure that the index was read
  Export also the has_un{staged,committed}_changed() functions
  wt-status: teach has_{unstaged,uncommitted}_changes() about submodules

 builtin/pull.c | 71 +++----------------------------------------------
 wt-status.c    | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h    |  5 ++++
 3 files changed, 91 insertions(+), 68 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/require-clean-work-tree-v1
Fetch-It-Via: git fetch https://github.com/dscho/git require-clean-work-tree-v1

-- 
2.10.0.rc1.99.gcd66998

base-commit: 2632c897f74b1cc9b5533f467da459b9ec725538

[PATCH 1/6] pull: drop confusing prefix parameter of die_on_unclean_work_tree()

[Johannes Schindelin]

In cmd_pull(), when verifying that there are no changes preventing a
rebasing pull, we diligently pass the prefix parameter to the
die_on_unclean_work_tree() function which in turn diligently passes it
to the has_unstaged_changes() and has_uncommitted_changes() functions.

The casual reader might now be curious (as this developer was) whether
that means that calling `git pull --rebase` in a subdirectory will
ignore unstaged changes in other parts of the working directory. And be
puzzled that `git pull --rebase` (correctly) complains about those
changes outside of the current directory.

The puzzle is easily resolved: while we take pains to pass around the
prefix and even pass it to init_revisions(), the fact that no paths are
passed to init_revisions() ensures that the prefix is simply ignored.

That, combined with the fact that we will *always* want a *full* working
directory check before running a rebasing pull, is reason enough to
simply do away with the actual prefix parameter and to pass NULL
instead, as if we were running this from the top-level working directory
anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 398aae1..d4bd635 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -328,12 +328,12 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-static int has_unstaged_changes(const char *prefix)
+static int has_unstaged_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
 
-	init_revisions(&rev_info, prefix);
+	init_revisions(&rev_info, NULL);
 	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	diff_setup_done(&rev_info.diffopt);
@@ -344,7 +344,7 @@ static int has_unstaged_changes(const char *prefix)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-static int has_uncommitted_changes(const char *prefix)
+static int has_uncommitted_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
@@ -352,7 +352,7 @@ static int has_uncommitted_changes(const char *prefix)
 	if (is_cache_unborn())
 		return 0;
 
-	init_revisions(&rev_info, prefix);
+	init_revisions(&rev_info, NULL);
 	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	add_head_to_pending(&rev_info);
@@ -365,7 +365,7 @@ static int has_uncommitted_changes(const char *prefix)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-static void die_on_unclean_work_tree(const char *prefix)
+static void die_on_unclean_work_tree(void)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
 	int do_die = 0;
@@ -375,12 +375,12 @@ static void die_on_unclean_work_tree(const char *prefix)
 	update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
-	if (has_unstaged_changes(prefix)) {
+	if (has_unstaged_changes()) {
 		error(_("Cannot pull with rebase: You have unstaged changes."));
 		do_die = 1;
 	}
 
-	if (has_uncommitted_changes(prefix)) {
+	if (has_uncommitted_changes()) {
 		if (do_die)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
@@ -875,7 +875,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Updating an unborn branch with changes added to the index."));
 
 		if (!autostash)
-			die_on_unclean_work_tree(prefix);
+			die_on_unclean_work_tree();
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 1/6] pull: drop confusing prefix parameter of die_on_unclean_work_tree()

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> -static int has_unstaged_changes(const char *prefix)
> +static int has_unstaged_changes(void)
>  {
>  	struct rev_info rev_info;
>  	int result;
>  
> -	init_revisions(&rev_info, prefix);
> +	init_revisions(&rev_info, NULL);
>  	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
>  	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
>  	diff_setup_done(&rev_info.diffopt);
> @@ -344,7 +344,7 @@ static int has_unstaged_changes(const char *prefix)
>  /**
>   * Returns 1 if there are uncommitted changes, 0 otherwise.
>   */
> -static int has_uncommitted_changes(const char *prefix)
> +static int has_uncommitted_changes(void)
>  {
>  	struct rev_info rev_info;
>  	int result;
> @@ -352,7 +352,7 @@ static int has_uncommitted_changes(const char *prefix)
>  	if (is_cache_unborn())
>  		return 0;
>  
> -	init_revisions(&rev_info, prefix);
> +	init_revisions(&rev_info, NULL);

I agree these are good changes.  The prefix being ignored is a mere
accident and a time-bomb waiting to go off.  As you mentioned in the
log message, we do not even want to pass a pathspec to limit the "is
there anything modified" check, which may be affected with prefix,
but we want a full-tree diff no matter where we started in these two
functions.

Good.

[PATCH 2/6] pull: make code more similar to the shell script again

[Johannes Schindelin]

When converting the pull command to a builtin, the
require_clean_work_tree() function was renamed and the pull-specific
parts hard-coded.

This makes it impossible to reuse the code, so let's modify the code to
make it more similar to the original shell script again.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d4bd635..4d1f9c8 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -365,10 +365,11 @@ static int has_uncommitted_changes(void)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-static void die_on_unclean_work_tree(void)
+static int require_clean_work_tree(const char *action, const char *hint,
+		int gently)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-	int do_die = 0;
+	int err = 0;
 
 	hold_locked_index(lock_file, 0);
 	refresh_cache(REFRESH_QUIET);
@@ -376,20 +377,26 @@ static void die_on_unclean_work_tree(void)
 	rollback_lock_file(lock_file);
 
 	if (has_unstaged_changes()) {
-		error(_("Cannot pull with rebase: You have unstaged changes."));
-		do_die = 1;
+		error(_("Cannot %s: You have unstaged changes."), action);
+		err = 1;
 	}
 
 	if (has_uncommitted_changes()) {
-		if (do_die)
+		if (err)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
-			error(_("Cannot pull with rebase: Your index contains uncommitted changes."));
-		do_die = 1;
+			error(_("Cannot %s: Your index contains uncommitted changes."), action);
+		err = 1;
 	}
 
-	if (do_die)
-		exit(1);
+	if (err) {
+		if (hint)
+			error("%s", hint);
+		if (!gently)
+			exit(err);
+	}
+
+	return err;
 }
 
 /**
@@ -875,7 +882,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Updating an unborn branch with changes added to the index."));
 
 		if (!autostash)
-			die_on_unclean_work_tree();
+			require_clean_work_tree("pull with rebase",
+				"Please commit or stash them.", 0);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 2/6] pull: make code more similar to the shell script again

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> +static int require_clean_work_tree(const char *action, const char *hint,
> +		int gently)
>  {
>  	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> -	int do_die = 0;
> +	int err = 0;
>  
>  	hold_locked_index(lock_file, 0);
>  	refresh_cache(REFRESH_QUIET);
> @@ -376,20 +377,26 @@ static void die_on_unclean_work_tree(void)
>  	rollback_lock_file(lock_file);
>  
>  	if (has_unstaged_changes()) {
> -		error(_("Cannot pull with rebase: You have unstaged changes."));
> -		do_die = 1;
> +		error(_("Cannot %s: You have unstaged changes."), action);
> ...
>  		if (!autostash)
> -			die_on_unclean_work_tree();
> +			require_clean_work_tree("pull with rebase",
> +				"Please commit or stash them.", 0);
>  
>  		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
>  			hashclr(rebase_fork_point);

Splicing an English/C phrase 'pull with rebase' into a
_("localizable %s string") makes the life of i18n team hard.

Can we do this differently?

If you are eventually going to expose this function as public API, I
think the right approach would be to enumerate the possible error
conditions this function can diagnose and return them to the caller,
i.e.

    #define WT_STATUS_DIRTY_WORKTREE 01
    #define WT_STATUS_DIRTY_INDEX    02

    static int require_clean_work_tree(void)
    {
	int status = 0;
	...
        if (has_unstaged_changes())
        	status |= WT_STATUS_DIRTY_WORKTREE;
	if (has_uncommitted_changes())
        	status |= WT_STATUS_DIRTY_INDEX;
	return status;
    }

Then die_on_unclean_work_tree() can be made as a thin-wrapper that
calls it and shows the pull-specific error message.

Re: [PATCH 2/6] pull: make code more similar to the shell script again

[Johannes Schindelin]

Hi Junio,

On Mon, 29 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +static int require_clean_work_tree(const char *action, const char *hint,
> > +		int gently)
> >  {
> >  	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> > -	int do_die = 0;
> > +	int err = 0;
> >  
> >  	hold_locked_index(lock_file, 0);
> >  	refresh_cache(REFRESH_QUIET);
> > @@ -376,20 +377,26 @@ static void die_on_unclean_work_tree(void)
> >  	rollback_lock_file(lock_file);
> >  
> >  	if (has_unstaged_changes()) {
> > -		error(_("Cannot pull with rebase: You have unstaged changes."));
> > -		do_die = 1;
> > +		error(_("Cannot %s: You have unstaged changes."), action);
> > ...
> >  		if (!autostash)
> > -			die_on_unclean_work_tree();
> > +			require_clean_work_tree("pull with rebase",
> > +				"Please commit or stash them.", 0);
> >  
> >  		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
> >  			hashclr(rebase_fork_point);
> 
> Splicing an English/C phrase 'pull with rebase' into a
> _("localizable %s string") makes the life of i18n team hard.

Hrm.

> Can we do this differently?

Sure, but not at this stage. Because...

> If you are eventually going to expose this function as public API, I
> think the right approach would be to enumerate the possible error
> conditions this function can diagnose and return them to the caller,
> i.e.
> 
>     #define WT_STATUS_DIRTY_WORKTREE 01
>     #define WT_STATUS_DIRTY_INDEX    02
> 
>     static int require_clean_work_tree(void)
>     {
> 	int status = 0;
> 	...
>         if (has_unstaged_changes())
>         	status |= WT_STATUS_DIRTY_WORKTREE;
> 	if (has_uncommitted_changes())
>         	status |= WT_STATUS_DIRTY_INDEX;
> 	return status;
>     }
> 
> Then die_on_unclean_work_tree() can be made as a thin-wrapper that
> calls it and shows the pull-specific error message.

This sounds like a good plan, if involved. At this stage, I am really
unwilling to introduce such extensive changes, for fear of introducing
regressions.

I will keep it in mind and make those changes, once Git for Windows
v2.10.0 is out.

Ciao,
Dscho

Re: [PATCH 2/6] pull: make code more similar to the shell script again

[Johannes Schindelin]

Hi Junio,

On Mon, 29 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > +static int require_clean_work_tree(const char *action, const char *hint,
> > +		int gently)
> >  {
> >  	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> > -	int do_die = 0;
> > +	int err = 0;
> >  
> >  	hold_locked_index(lock_file, 0);
> >  	refresh_cache(REFRESH_QUIET);
> > @@ -376,20 +377,26 @@ static void die_on_unclean_work_tree(void)
> >  	rollback_lock_file(lock_file);
> >  
> >  	if (has_unstaged_changes()) {
> > -		error(_("Cannot pull with rebase: You have unstaged changes."));
> > -		do_die = 1;
> > +		error(_("Cannot %s: You have unstaged changes."), action);
> > ...
> >  		if (!autostash)
> > -			die_on_unclean_work_tree();
> > +			require_clean_work_tree("pull with rebase",
> > +				"Please commit or stash them.", 0);
> >  
> >  		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
> >  			hashclr(rebase_fork_point);
> 
> Splicing an English/C phrase 'pull with rebase' into a
> _("localizable %s string") makes the life of i18n team hard.
> 
> Can we do this differently?
> 
> If you are eventually going to expose this function as public API, I
> think the right approach would be to enumerate the possible error
> conditions this function can diagnose and return them to the caller,
> i.e.
> 
>     #define WT_STATUS_DIRTY_WORKTREE 01
>     #define WT_STATUS_DIRTY_INDEX    02
> 
>     static int require_clean_work_tree(void)
>     {
> 	int status = 0;
> 	...
>         if (has_unstaged_changes())
>         	status |= WT_STATUS_DIRTY_WORKTREE;
> 	if (has_uncommitted_changes())
>         	status |= WT_STATUS_DIRTY_INDEX;
> 	return status;
>     }
> 
> Then die_on_unclean_work_tree() can be made as a thin-wrapper that
> calls it and shows the pull-specific error message.

Hrm. After thinking about this for over a week, I think that this is the
wrong approach.

To introduce new wrapper functions just for the sake of being able to
provide possibly dozens of different error messages seems quite a bit
wrong.

I agree, however, that it may be a better idea to add a "gently" flag to
require_clean_work_tree() that lets it print out a (localizable) error
message and return -1 instead of die()ing.

The result would be that a failed `git pull --rebase` would then print out
*two* lines: one explaining that there are unstaged changes, and one that
explains that the pull did not even start due to an unclean work tree.

That solution would scale better, and I may get a chance to make those
changes and send out another iteration of this patch series before
October.

Ciao,
Johannes

[PATCH 3/6] Make the require_clean_work_tree() function truly reusable

[Johannes Schindelin]

It is remarkable that libgit.a did not sport this function yet... Let's
move it into a more prominent (and into an actually reusable) spot:
wt-status.[ch].

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 75 +---------------------------------------------------------
 wt-status.c    | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h    |  2 ++
 3 files changed, 77 insertions(+), 74 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 4d1f9c8..c35c6e8 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -17,6 +17,7 @@
 #include "revision.h"
 #include "tempfile.h"
 #include "lockfile.h"
+#include "wt-status.h"
 
 enum rebase_type {
 	REBASE_INVALID = -1,
@@ -326,80 +327,6 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 }
 
 /**
- * Returns 1 if there are unstaged changes, 0 otherwise.
- */
-static int has_unstaged_changes(void)
-{
-	struct rev_info rev_info;
-	int result;
-
-	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_files(&rev_info, 0);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * Returns 1 if there are uncommitted changes, 0 otherwise.
- */
-static int has_uncommitted_changes(void)
-{
-	struct rev_info rev_info;
-	int result;
-
-	if (is_cache_unborn())
-		return 0;
-
-	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	add_head_to_pending(&rev_info);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_index(&rev_info, 1);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * If the work tree has unstaged or uncommitted changes, dies with the
- * appropriate message.
- */
-static int require_clean_work_tree(const char *action, const char *hint,
-		int gently)
-{
-	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-	int err = 0;
-
-	hold_locked_index(lock_file, 0);
-	refresh_cache(REFRESH_QUIET);
-	update_index_if_able(&the_index, lock_file);
-	rollback_lock_file(lock_file);
-
-	if (has_unstaged_changes()) {
-		error(_("Cannot %s: You have unstaged changes."), action);
-		err = 1;
-	}
-
-	if (has_uncommitted_changes()) {
-		if (err)
-			error(_("Additionally, your index contains uncommitted changes."));
-		else
-			error(_("Cannot %s: Your index contains uncommitted changes."), action);
-		err = 1;
-	}
-
-	if (err) {
-		if (hint)
-			error("%s", hint);
-		if (!gently)
-			exit(err);
-	}
-
-	return err;
-}
-
-/**
  * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
  * into merge_heads.
  */
diff --git a/wt-status.c b/wt-status.c
index 6225a2d..792dda9 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,7 @@
 #include "strbuf.h"
 #include "utf8.h"
 #include "worktree.h"
+#include "lockfile.h"
 
 static const char cut_line[] =
 "------------------------ >8 ------------------------\n";
@@ -1757,3 +1758,76 @@ void wt_porcelain_print(struct wt_status *s)
 	s->no_gettext = 1;
 	wt_shortstatus_print(s);
 }
+
+/**
+ * Returns 1 if there are unstaged changes, 0 otherwise.
+ */
+static int has_unstaged_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_files(&rev_info, 0);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * Returns 1 if there are uncommitted changes, 0 otherwise.
+ */
+static int has_uncommitted_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	if (is_cache_unborn())
+		return 0;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	add_head_to_pending(&rev_info);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_index(&rev_info, 1);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * If the work tree has unstaged or uncommitted changes, dies with the
+ * appropriate message.
+ */
+int require_clean_work_tree(const char *action, const char *hint, int gently)
+{
+	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
+	int err = 0;
+
+	hold_locked_index(lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	update_index_if_able(&the_index, lock_file);
+	rollback_lock_file(lock_file);
+
+	if (has_unstaged_changes()) {
+		error(_("Cannot %s: You have unstaged changes."), action);
+		err = 1;
+	}
+
+	if (has_uncommitted_changes()) {
+		if (err)
+			error(_("Additionally, your index contains uncommitted changes."));
+		else
+			error(_("Cannot %s: Your index contains uncommitted changes."), action);
+		err = 1;
+	}
+
+	if (err) {
+		if (hint)
+			error("%s", hint);
+		if (!gently)
+			exit(err);
+	}
+
+	return err;
+}
diff --git a/wt-status.h b/wt-status.h
index 2ca93f6..cc4e5a3 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -115,4 +115,6 @@ void status_printf_ln(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, ...);
 
+int require_clean_work_tree(const char *action, const char *hint, int gently);
+
 #endif /* STATUS_H */
-- 
2.10.0.rc1.99.gcd66998

[PATCH 4/6] require_clean_work_tree: ensure that the index was read

[Johannes Schindelin]

The function would otherwise pretend to work fine, but totally ignore
the working directory.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/wt-status.c b/wt-status.c
index 792dda9..33dd76f 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1804,6 +1804,13 @@ int require_clean_work_tree(const char *action, const char *hint, int gently)
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
 	int err = 0;
 
+	if (read_cache() < 0) {
+		error(_("Could not read index"));
+		if (gently)
+			return -1;
+		exit(1);
+	}
+
 	hold_locked_index(lock_file, 0);
 	refresh_cache(REFRESH_QUIET);
 	update_index_if_able(&the_index, lock_file);
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 4/6] require_clean_work_tree: ensure that the index was read

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> The function would otherwise pretend to work fine, but totally ignore
> the working directory.

s/^T/Unless the caller has already read the index, t/;

I am not sure if it should be left as the responsibility of the
caller (i.e. check the_index.initialized to bark at a caller that
forgets to read from an index) or it is OK to unconditionally read
from $GIT_INDEX_FILE in a truly libified function.  A caller that
wants to read from a custom (temporary) index file can choose to
make sure it does read_inddex_from() from its custom file before
calling this function, but if it forgets to do so, the penalty is
severe than ordinary callers who would have read from the normal
index file anyway.

Even though the word-lego issue would make this particular API not
yet suitable, "who is responsible for reading the index" is an
orthogonal issue that we can discuss even on this version, so I
thought I'd bring it up.

>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  wt-status.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/wt-status.c b/wt-status.c
> index 792dda9..33dd76f 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1804,6 +1804,13 @@ int require_clean_work_tree(const char *action, const char *hint, int gently)
>  	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
>  	int err = 0;
>  
> +	if (read_cache() < 0) {
> +		error(_("Could not read index"));
> +		if (gently)
> +			return -1;
> +		exit(1);
> +	}
> +
>  	hold_locked_index(lock_file, 0);
>  	refresh_cache(REFRESH_QUIET);
>  	update_index_if_able(&the_index, lock_file);

Re: [PATCH 4/6] require_clean_work_tree: ensure that the index was read

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I am not sure if it should be left as the responsibility of the
> caller (i.e. check the_index.initialized to bark at a caller that
> forgets to read from an index) ...

Scatch that.  That would not work in a freshly created repository
before doing any "git add".  An empty index is a normal state, so it
would not just be annoying to warn "You called me without reading
the index" but is simply wrong.

It was OK to have "we ensure that we've read into the_index from
some index file" as a fallback in a helper function designed to be
used in a "run once and exit" program, but I'd say as a part of
"libified" helper set, we should just make it a responsibility of
the caller to make sure whatever index entries the caller wants to
have in the_index to be used when calling this helper function.

During the course of developing whatever you are building that calls
this function, perhaps you were bitten by an unpolulated the_index,
_not_ because you genuinely could _not_ find the logical place that
is the best location to read the index file at in your code flow,
but simply because you forgot to read one and with that hindsight,
you _know_ what is the logical right place the index file should
have been read.  That is what I am guessing anyway.

And I further guess that this is a well-meaning attempt to _help_
others not having to worry about _when_ exactly the index file is
read.

But I do not think it is being helpful in the longer term.  When to
read the index file and when to discard the contents matters (and
for callers to which it does not matter, they can always read it at
the very beginning of the program, because by definition it does not
matter).  So let's not do this patch, unless there is some other
good reason to have it.

Re: [PATCH 4/6] require_clean_work_tree: ensure that the index was read

[Johannes Schindelin]

Hi Junio,

On Mon, 29 Aug 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > I am not sure if it should be left as the responsibility of the
> > caller (i.e. check the_index.initialized to bark at a caller that
> > forgets to read from an index) ...
> 
> Scatch that.  That would not work in a freshly created repository
> before doing any "git add".  An empty index is a normal state, so it
> would not just be annoying to warn "You called me without reading
> the index" but is simply wrong.

Fine. I changed it to assert that the_index.initialized was set.

Ciao,
Dscho

Re: [PATCH 4/6] require_clean_work_tree: ensure that the index was read

[Johannes Schindelin]

Hi Junio,

On Tue, 30 Aug 2016, Johannes Schindelin wrote:

> On Mon, 29 Aug 2016, Junio C Hamano wrote:
> 
> > Junio C Hamano <gitster@pobox.com> writes:
> > 
> > > I am not sure if it should be left as the responsibility of the
> > > caller (i.e. check the_index.initialized to bark at a caller that
> > > forgets to read from an index) ...
> > 
> > Scatch that.  That would not work in a freshly created repository
> > before doing any "git add".  An empty index is a normal state, so it
> > would not just be annoying to warn "You called me without reading
> > the index" but is simply wrong.
> 
> Fine. I changed it to assert that the_index.initialized was set.

Alas, that does not work, either. If no .git/index exists, read_index()
will not set the "initialized" flag.

So it turns out that I can either get distracted in a major way, or drop
the patch. I opt for the latter.

Ciao,
Dscho

Re: [PATCH 4/6] require_clean_work_tree: ensure that the index was read

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> > Scatch that.  That would not work in a freshly created repository
>> > before doing any "git add".  An empty index is a normal state,...
>
> Alas, that does not work, either. If no .git/index exists, read_index()
> will not set the "initialized" flag.

Exactly

Re: [PATCH 4/6] require_clean_work_tree: ensure that the index was read

[Johannes Schindelin]

Hi Junio,

On Mon, 29 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > The function would otherwise pretend to work fine, but totally ignore
> > the working directory.
> 
> s/^T/Unless the caller has already read the index, t/;

Changed. (I also removed the "otherwise" which would sound funny
otherwise.)

> I am not sure if it should be left as the responsibility of the
> caller (i.e. check the_index.initialized to bark at a caller that
> forgets to read from an index) or it is OK to unconditionally read
> from $GIT_INDEX_FILE in a truly libified function.  A caller that
> wants to read from a custom (temporary) index file can choose to
> make sure it does read_inddex_from() from its custom file before
> calling this function, but if it forgets to do so, the penalty is
> severe than ordinary callers who would have read from the normal
> index file anyway.
> 
> Even though the word-lego issue would make this particular API not
> yet suitable, "who is responsible for reading the index" is an
> orthogonal issue that we can discuss even on this version, so I
> thought I'd bring it up.

It is orthogonal alright. So I won't act on it, but just add my thoughts:

We might want to consider thinking about introducing a more consistent
internal API. This is even more orthogonal to the patch under discussion
than just teaching require_clean_work_tree() about different index files,
of course.

It might very well pay off in the future were we to design a more unified
"look & feel" to the API e.g. by putting more restrictions on the order of
parameters, required "int" return values for functions that may fail, a
unified error handling that does not necessarily print to stderr.

Having said that, I do not have time to take lead on that, either. :-)

Ciao,
Dscho

[PATCH 5/6] Export also the has_un{staged,committed}_changed() functions

[Johannes Schindelin]

They will be used in the upcoming rebase helper.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 4 ++--
 wt-status.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 33dd76f..1c3fabf 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1762,7 +1762,7 @@ void wt_porcelain_print(struct wt_status *s)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-static int has_unstaged_changes(void)
+int has_unstaged_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
@@ -1778,7 +1778,7 @@ static int has_unstaged_changes(void)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-static int has_uncommitted_changes(void)
+int has_uncommitted_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
diff --git a/wt-status.h b/wt-status.h
index cc4e5a3..75833c1 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -115,6 +115,8 @@ void status_printf_ln(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, ...);
 
+int has_unstaged_changes(void);
+int has_uncommitted_changes(void);
 int require_clean_work_tree(const char *action, const char *hint, int gently);
 
 #endif /* STATUS_H */
-- 
2.10.0.rc1.99.gcd66998

Re: [PATCH 5/6] Export also the has_un{staged,committed}_changed() functions

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> diff --git a/wt-status.h b/wt-status.h
> index cc4e5a3..75833c1 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -115,6 +115,8 @@ void status_printf_ln(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, ...);
>  
> +int has_unstaged_changes(void);
> +int has_uncommitted_changes(void);
>  int require_clean_work_tree(const char *action, const char *hint, int gently);
>  
>  #endif /* STATUS_H */

These are good interfaces, if the caller wants to know these bits
independently.

[PATCH 6/6] wt-status: teach has_{unstaged,uncommitted}_changes() about submodules

[Johannes Schindelin]

Sometimes we are *actually* interested in those changes... For
example when an interactive rebase wants to continue with a staged
submodule update.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c |  2 +-
 wt-status.c    | 16 +++++++++-------
 wt-status.h    |  7 ++++---
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index c35c6e8..843ff19 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 		if (!autostash)
 			require_clean_work_tree("pull with rebase",
-				"Please commit or stash them.", 0);
+				"Please commit or stash them.", 1, 0);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
diff --git a/wt-status.c b/wt-status.c
index 1c3fabf..8ba0b4d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1762,13 +1762,14 @@ void wt_porcelain_print(struct wt_status *s)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-int has_unstaged_changes(void)
+int has_unstaged_changes(int ignore_submodules)
 {
 	struct rev_info rev_info;
 	int result;
 
 	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	if (ignore_submodules)
+		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	diff_setup_done(&rev_info.diffopt);
 	result = run_diff_files(&rev_info, 0);
@@ -1778,7 +1779,7 @@ int has_unstaged_changes(void)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-int has_uncommitted_changes(void)
+int has_uncommitted_changes(int ignore_submodules)
 {
 	struct rev_info rev_info;
 	int result;
@@ -1787,7 +1788,8 @@ int has_uncommitted_changes(void)
 		return 0;
 
 	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	if (ignore_submodules)
+		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	add_head_to_pending(&rev_info);
 	diff_setup_done(&rev_info.diffopt);
@@ -1799,7 +1801,7 @@ int has_uncommitted_changes(void)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-int require_clean_work_tree(const char *action, const char *hint, int gently)
+int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
 	int err = 0;
@@ -1816,12 +1818,12 @@ int require_clean_work_tree(const char *action, const char *hint, int gently)
 	update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
-	if (has_unstaged_changes()) {
+	if (has_unstaged_changes(ignore_submodules)) {
 		error(_("Cannot %s: You have unstaged changes."), action);
 		err = 1;
 	}
 
-	if (has_uncommitted_changes()) {
+	if (has_uncommitted_changes(ignore_submodules)) {
 		if (err)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
diff --git a/wt-status.h b/wt-status.h
index 75833c1..ba56c01 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -115,8 +115,9 @@ void status_printf_ln(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, ...);
 
-int has_unstaged_changes(void);
-int has_uncommitted_changes(void);
-int require_clean_work_tree(const char *action, const char *hint, int gently);
+int has_unstaged_changes(int ignore_submodules);
+int has_uncommitted_changes(int ignore_submodules);
+int require_clean_work_tree(const char *action, const char *hint,
+	int ignore_submodules, int gently);
 
 #endif /* STATUS_H */
-- 
2.10.0.rc1.99.gcd66998

[PATCH v2 0/5] Pull out require_clean_work_tree() functionality from builtin/pull.c

[Johannes Schindelin]

This is the 5th last patch series of my work to accelerate interactive
rebases in particular on Windows.

Basically, all it does is to make reusable some functions that were
ported over from git-pull.sh but made private to builtin/pull.c.

Changes since v1:

- skipped patch that tries to make require_clean_work_tree() smart
  enough to read the index if it was not read yet.

- added a code-comment clarifying that it is the duty of
  require_clean_work_tree()'s caller to ensure that the index has been
  read.

- made the action in require_clean_work_tree() translateable. This
  amazingly easy without complexifying the code, simply by using N_()
  and _() as indicated by Jakub.


Johannes Schindelin (5):
  pull: drop confusing prefix parameter of die_on_unclean_work_tree()
  pull: make code more similar to the shell script again
  Make the require_clean_work_tree() function truly reusable
  Export also the has_un{staged,committed}_changed() functions
  wt-status: teach has_{unstaged,uncommitted}_changes() about submodules

 builtin/pull.c | 71 +++--------------------------------------------------
 wt-status.c    | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h    |  6 +++++
 3 files changed, 86 insertions(+), 68 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/require-clean-work-tree-v2
Fetch-It-Via: git fetch https://github.com/dscho/git require-clean-work-tree-v2

Interdiff vs v1:

 diff --git a/builtin/pull.c b/builtin/pull.c
 index 843ff19..c639167 100644
 --- a/builtin/pull.c
 +++ b/builtin/pull.c
 @@ -809,7 +809,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
  			die(_("Updating an unborn branch with changes added to the index."));
  
  		if (!autostash)
 -			require_clean_work_tree("pull with rebase",
 +			require_clean_work_tree(N_("pull with rebase"),
  				"Please commit or stash them.", 1, 0);
  
  		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 diff --git a/wt-status.c b/wt-status.c
 index 129b054..086ae79 100644
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -2258,20 +2258,13 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub
  	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
  	int err = 0;
  
 -	if (read_cache() < 0) {
 -		error(_("Could not read index"));
 -		if (gently)
 -			return -1;
 -		exit(1);
 -	}
 -
  	hold_locked_index(lock_file, 0);
  	refresh_cache(REFRESH_QUIET);
  	update_index_if_able(&the_index, lock_file);
  	rollback_lock_file(lock_file);
  
  	if (has_unstaged_changes(ignore_submodules)) {
 -		error(_("Cannot %s: You have unstaged changes."), action);
 +		error(_("Cannot %s: You have unstaged changes."), _(action));
  		err = 1;
  	}
  
 @@ -2279,7 +2272,8 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub
  		if (err)
  			error(_("Additionally, your index contains uncommitted changes."));
  		else
 -			error(_("Cannot %s: Your index contains uncommitted changes."), action);
 +			error(_("Cannot %s: Your index contains uncommitted changes."),
 +			      _(action));
  		err = 1;
  	}
  
 diff --git a/wt-status.h b/wt-status.h
 index f0e66c4..54fec77 100644
 --- a/wt-status.h
 +++ b/wt-status.h
 @@ -128,6 +128,7 @@ void status_printf_ln(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, ...);
  
 +/* The following functions expect that the caller took care of reading the index. */
  int has_unstaged_changes(int ignore_submodules);
  int has_uncommitted_changes(int ignore_submodules);
  int require_clean_work_tree(const char *action, const char *hint,

-- 
2.10.0.windows.1.10.g803177d

base-commit: cda1bbd474805e653dda8a71d4ea3790e2a66cbb

[PATCH v2 1/5] pull: drop confusing prefix parameter of die_on_unclean_work_tree()

[Johannes Schindelin]

In cmd_pull(), when verifying that there are no changes preventing a
rebasing pull, we diligently pass the prefix parameter to the
die_on_unclean_work_tree() function which in turn diligently passes it
to the has_unstaged_changes() and has_uncommitted_changes() functions.

The casual reader might now be curious (as this developer was) whether
that means that calling `git pull --rebase` in a subdirectory will
ignore unstaged changes in other parts of the working directory. And be
puzzled that `git pull --rebase` (correctly) complains about those
changes outside of the current directory.

The puzzle is easily resolved: while we take pains to pass around the
prefix and even pass it to init_revisions(), the fact that no paths are
passed to init_revisions() ensures that the prefix is simply ignored.

That, combined with the fact that we will *always* want a *full* working
directory check before running a rebasing pull, is reason enough to
simply do away with the actual prefix parameter and to pass NULL
instead, as if we were running this from the top-level working directory
anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 398aae1..d4bd635 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -328,12 +328,12 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-static int has_unstaged_changes(const char *prefix)
+static int has_unstaged_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
 
-	init_revisions(&rev_info, prefix);
+	init_revisions(&rev_info, NULL);
 	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	diff_setup_done(&rev_info.diffopt);
@@ -344,7 +344,7 @@ static int has_unstaged_changes(const char *prefix)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-static int has_uncommitted_changes(const char *prefix)
+static int has_uncommitted_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
@@ -352,7 +352,7 @@ static int has_uncommitted_changes(const char *prefix)
 	if (is_cache_unborn())
 		return 0;
 
-	init_revisions(&rev_info, prefix);
+	init_revisions(&rev_info, NULL);
 	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	add_head_to_pending(&rev_info);
@@ -365,7 +365,7 @@ static int has_uncommitted_changes(const char *prefix)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-static void die_on_unclean_work_tree(const char *prefix)
+static void die_on_unclean_work_tree(void)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
 	int do_die = 0;
@@ -375,12 +375,12 @@ static void die_on_unclean_work_tree(const char *prefix)
 	update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
-	if (has_unstaged_changes(prefix)) {
+	if (has_unstaged_changes()) {
 		error(_("Cannot pull with rebase: You have unstaged changes."));
 		do_die = 1;
 	}
 
-	if (has_uncommitted_changes(prefix)) {
+	if (has_uncommitted_changes()) {
 		if (do_die)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
@@ -875,7 +875,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Updating an unborn branch with changes added to the index."));
 
 		if (!autostash)
-			die_on_unclean_work_tree(prefix);
+			die_on_unclean_work_tree();
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-- 
2.10.0.windows.1.10.g803177d

[PATCH v2 2/5] pull: make code more similar to the shell script again

[Johannes Schindelin]

When converting the pull command to a builtin, the
require_clean_work_tree() function was renamed and the pull-specific
parts hard-coded.

This makes it impossible to reuse the code, so let's modify the code to
make it more similar to the original shell script again.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d4bd635..a3ed054 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -365,10 +365,11 @@ static int has_uncommitted_changes(void)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-static void die_on_unclean_work_tree(void)
+static int require_clean_work_tree(const char *action, const char *hint,
+		int gently)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-	int do_die = 0;
+	int err = 0;
 
 	hold_locked_index(lock_file, 0);
 	refresh_cache(REFRESH_QUIET);
@@ -376,20 +377,27 @@ static void die_on_unclean_work_tree(void)
 	rollback_lock_file(lock_file);
 
 	if (has_unstaged_changes()) {
-		error(_("Cannot pull with rebase: You have unstaged changes."));
-		do_die = 1;
+		error(_("Cannot %s: You have unstaged changes."), _(action));
+		err = 1;
 	}
 
 	if (has_uncommitted_changes()) {
-		if (do_die)
+		if (err)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
-			error(_("Cannot pull with rebase: Your index contains uncommitted changes."));
-		do_die = 1;
+			error(_("Cannot %s: Your index contains uncommitted changes."),
+			      _(action));
+		err = 1;
 	}
 
-	if (do_die)
-		exit(1);
+	if (err) {
+		if (hint)
+			error("%s", hint);
+		if (!gently)
+			exit(err);
+	}
+
+	return err;
 }
 
 /**
@@ -875,7 +883,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Updating an unborn branch with changes added to the index."));
 
 		if (!autostash)
-			die_on_unclean_work_tree();
+			require_clean_work_tree(N_("pull with rebase"),
+				"Please commit or stash them.", 0);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-- 
2.10.0.windows.1.10.g803177d

Re: [PATCH v2 2/5] pull: make code more similar to the shell script again

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> When converting the pull command to a builtin, the
> require_clean_work_tree() function was renamed and the pull-specific
> parts hard-coded.
>
> This makes it impossible to reuse the code, so let's modify the code to
> make it more similar to the original shell script again.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/pull.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index d4bd635..a3ed054 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -365,10 +365,11 @@ static int has_uncommitted_changes(void)
>   * If the work tree has unstaged or uncommitted changes, dies with the
>   * appropriate message.
>   */
> -static void die_on_unclean_work_tree(void)
> +static int require_clean_work_tree(const char *action, const char *hint,
> +		int gently)
>  {
>  	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
> -	int do_die = 0;
> +	int err = 0;
>  
>  	hold_locked_index(lock_file, 0);
>  	refresh_cache(REFRESH_QUIET);
> @@ -376,20 +377,27 @@ static void die_on_unclean_work_tree(void)
>  	rollback_lock_file(lock_file);
>  
>  	if (has_unstaged_changes()) {
> -		error(_("Cannot pull with rebase: You have unstaged changes."));
> -		do_die = 1;
> +		error(_("Cannot %s: You have unstaged changes."), _(action));
> +		err = 1;
>  	}
> ...
> +			error(_("Cannot %s: Your index contains uncommitted changes."),
> +			      _(action));
> +		err = 1;

These are much better than the one in v1.

Depending on the target language, the translators may have to phrase
these not like "Cannot <verb>:" but "Cannot perform <noun>:" where
the "<noun>" is for "the act of doing <verb>", if the "cannot" part
in their language needs to change shape depending on the verb.
Hence, I think the translators need a /* TRANSLATORS: ... */ comment
that tells them what is interpolated here are their translations for
phrases like "pull with rebase".  You do not have to be exhausitive
in the comment; a representative example would help the translators
see the message in context.

Other than that (and the need to further clean-up error() and die()
to begin with lower-case to match the modern practice in a separate
follow-up series), this looks ready to be queued.

Thanks.

[PATCH v2 3/5] Make the require_clean_work_tree() function truly reusable

[Johannes Schindelin]

It is remarkable that libgit.a did not sport this function yet... Let's
move it into a more prominent (and into an actually reusable) spot:
wt-status.[ch].

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 76 +---------------------------------------------------------
 wt-status.c    | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h    |  3 +++
 3 files changed, 79 insertions(+), 75 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index a3ed054..14ef8b5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -17,6 +17,7 @@
 #include "revision.h"
 #include "tempfile.h"
 #include "lockfile.h"
+#include "wt-status.h"
 
 enum rebase_type {
 	REBASE_INVALID = -1,
@@ -326,81 +327,6 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 }
 
 /**
- * Returns 1 if there are unstaged changes, 0 otherwise.
- */
-static int has_unstaged_changes(void)
-{
-	struct rev_info rev_info;
-	int result;
-
-	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_files(&rev_info, 0);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * Returns 1 if there are uncommitted changes, 0 otherwise.
- */
-static int has_uncommitted_changes(void)
-{
-	struct rev_info rev_info;
-	int result;
-
-	if (is_cache_unborn())
-		return 0;
-
-	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	add_head_to_pending(&rev_info);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_index(&rev_info, 1);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * If the work tree has unstaged or uncommitted changes, dies with the
- * appropriate message.
- */
-static int require_clean_work_tree(const char *action, const char *hint,
-		int gently)
-{
-	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-	int err = 0;
-
-	hold_locked_index(lock_file, 0);
-	refresh_cache(REFRESH_QUIET);
-	update_index_if_able(&the_index, lock_file);
-	rollback_lock_file(lock_file);
-
-	if (has_unstaged_changes()) {
-		error(_("Cannot %s: You have unstaged changes."), _(action));
-		err = 1;
-	}
-
-	if (has_uncommitted_changes()) {
-		if (err)
-			error(_("Additionally, your index contains uncommitted changes."));
-		else
-			error(_("Cannot %s: Your index contains uncommitted changes."),
-			      _(action));
-		err = 1;
-	}
-
-	if (err) {
-		if (hint)
-			error("%s", hint);
-		if (!gently)
-			exit(err);
-	}
-
-	return err;
-}
-
-/**
  * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
  * into merge_heads.
  */
diff --git a/wt-status.c b/wt-status.c
index 539aac1..9ab9adc 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,7 @@
 #include "strbuf.h"
 #include "utf8.h"
 #include "worktree.h"
+#include "lockfile.h"
 
 static const char cut_line[] =
 "------------------------ >8 ------------------------\n";
@@ -2209,3 +2210,77 @@ void wt_status_print(struct wt_status *s)
 		break;
 	}
 }
+
+/**
+ * Returns 1 if there are unstaged changes, 0 otherwise.
+ */
+static int has_unstaged_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_files(&rev_info, 0);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * Returns 1 if there are uncommitted changes, 0 otherwise.
+ */
+static int has_uncommitted_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	if (is_cache_unborn())
+		return 0;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	add_head_to_pending(&rev_info);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_index(&rev_info, 1);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * If the work tree has unstaged or uncommitted changes, dies with the
+ * appropriate message.
+ */
+int require_clean_work_tree(const char *action, const char *hint, int gently)
+{
+	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
+	int err = 0;
+
+	hold_locked_index(lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	update_index_if_able(&the_index, lock_file);
+	rollback_lock_file(lock_file);
+
+	if (has_unstaged_changes()) {
+		error(_("Cannot %s: You have unstaged changes."), _(action));
+		err = 1;
+	}
+
+	if (has_uncommitted_changes()) {
+		if (err)
+			error(_("Additionally, your index contains uncommitted changes."));
+		else
+			error(_("Cannot %s: Your index contains uncommitted changes."),
+			      _(action));
+		err = 1;
+	}
+
+	if (err) {
+		if (hint)
+			error("%s", hint);
+		if (!gently)
+			exit(err);
+	}
+
+	return err;
+}
diff --git a/wt-status.h b/wt-status.h
index e401837..03ecf53 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,4 +128,7 @@ void status_printf_ln(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, ...);
 
+/* The following function expect that the caller took care of reading the index. */
+int require_clean_work_tree(const char *action, const char *hint, int gently);
+
 #endif /* STATUS_H */
-- 
2.10.0.windows.1.10.g803177d

Re: [PATCH v2 3/5] Make the require_clean_work_tree() function truly reusable

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> It is remarkable that libgit.a did not sport this function yet... Let's
> move it into a more prominent (and into an actually reusable) spot:
> wt-status.[ch].
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

I do not think "truly" is needed at all.

It was not reusable.  It is somewhat reusable after this patch.  It
may or may not be "truly" reusable depending on the need of future
patches, which we do not know yet.

I agree wt-status.[ch] is a good home for this feature.

Thanks.

[PATCH v2 4/5] Export also the has_un{staged,committed}_changed() functions

[Johannes Schindelin]

They will be used in the upcoming rebase helper.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 4 ++--
 wt-status.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 9ab9adc..f1120e6 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2214,7 +2214,7 @@ void wt_status_print(struct wt_status *s)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-static int has_unstaged_changes(void)
+int has_unstaged_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
@@ -2230,7 +2230,7 @@ static int has_unstaged_changes(void)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-static int has_uncommitted_changes(void)
+int has_uncommitted_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
diff --git a/wt-status.h b/wt-status.h
index 03ecf53..68e367a 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,7 +128,9 @@ void status_printf_ln(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, ...);
 
-/* The following function expect that the caller took care of reading the index. */
+/* The following functions expect that the caller took care of reading the index. */
+int has_unstaged_changes(void);
+int has_uncommitted_changes(void);
 int require_clean_work_tree(const char *action, const char *hint, int gently);
 
 #endif /* STATUS_H */
-- 
2.10.0.windows.1.10.g803177d

[PATCH v2 5/5] wt-status: teach has_{unstaged,uncommitted}_changes() about submodules

[Johannes Schindelin]

Sometimes we are *actually* interested in those changes... For
example when an interactive rebase wants to continue with a staged
submodule update.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c |  2 +-
 wt-status.c    | 16 +++++++++-------
 wt-status.h    |  7 ++++---
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 14ef8b5..c639167 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 		if (!autostash)
 			require_clean_work_tree(N_("pull with rebase"),
-				"Please commit or stash them.", 0);
+				"Please commit or stash them.", 1, 0);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
diff --git a/wt-status.c b/wt-status.c
index f1120e6..086ae79 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2214,13 +2214,14 @@ void wt_status_print(struct wt_status *s)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-int has_unstaged_changes(void)
+int has_unstaged_changes(int ignore_submodules)
 {
 	struct rev_info rev_info;
 	int result;
 
 	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	if (ignore_submodules)
+		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	diff_setup_done(&rev_info.diffopt);
 	result = run_diff_files(&rev_info, 0);
@@ -2230,7 +2231,7 @@ int has_unstaged_changes(void)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-int has_uncommitted_changes(void)
+int has_uncommitted_changes(int ignore_submodules)
 {
 	struct rev_info rev_info;
 	int result;
@@ -2239,7 +2240,8 @@ int has_uncommitted_changes(void)
 		return 0;
 
 	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	if (ignore_submodules)
+		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	add_head_to_pending(&rev_info);
 	diff_setup_done(&rev_info.diffopt);
@@ -2251,7 +2253,7 @@ int has_uncommitted_changes(void)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-int require_clean_work_tree(const char *action, const char *hint, int gently)
+int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
 	int err = 0;
@@ -2261,12 +2263,12 @@ int require_clean_work_tree(const char *action, const char *hint, int gently)
 	update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
-	if (has_unstaged_changes()) {
+	if (has_unstaged_changes(ignore_submodules)) {
 		error(_("Cannot %s: You have unstaged changes."), _(action));
 		err = 1;
 	}
 
-	if (has_uncommitted_changes()) {
+	if (has_uncommitted_changes(ignore_submodules)) {
 		if (err)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
diff --git a/wt-status.h b/wt-status.h
index 68e367a..54fec77 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -129,8 +129,9 @@ __attribute__((format (printf, 3, 4)))
 void status_printf(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(void);
-int has_uncommitted_changes(void);
-int require_clean_work_tree(const char *action, const char *hint, int gently);
+int has_unstaged_changes(int ignore_submodules);
+int has_uncommitted_changes(int ignore_submodules);
+int require_clean_work_tree(const char *action, const char *hint,
+	int ignore_submodules, int gently);
 
 #endif /* STATUS_H */
-- 
2.10.0.windows.1.10.g803177d

Re: [PATCH v2 0/5] Pull out require_clean_work_tree() functionality from builtin/pull.c

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Johannes Schindelin (5):
>   pull: drop confusing prefix parameter of die_on_unclean_work_tree()
>   pull: make code more similar to the shell script again
>   Make the require_clean_work_tree() function truly reusable
>   Export also the has_un{staged,committed}_changed() functions
>   wt-status: teach has_{unstaged,uncommitted}_changes() about submodules

Other than two minor things I've already mentioned, this round looks
ready to be queued.  Thanks.

[PATCH v3 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c

[Johannes Schindelin]

This is the 5th last patch series of my work to accelerate interactive
rebases in particular on Windows.

Basically, all it does is to make reusable some functions that were
ported over from git-pull.sh but made private to builtin/pull.c.

Changes since v2:

- added a hint for translators.

- changed the existing error messages to start with a lower-case, as per
  our current convention (the previous error messages were inherited
  from code written before that convention was in place).

- struck the "truly" adjective from the commit message, as it did not
  get Junio's consent.


Johannes Schindelin (6):
  pull: drop confusing prefix parameter of die_on_unclean_work_tree()
  pull: make code more similar to the shell script again
  Make the require_clean_work_tree() function reusable
  Export also the has_un{staged,committed}_changed() functions
  wt-status: teach has_{unstaged,uncommitted}_changes() about submodules
  wt-status: begin error messages with lower-case

 builtin/pull.c | 71 +++-------------------------------------------------
 wt-status.c    | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h    |  6 +++++
 3 files changed, 87 insertions(+), 68 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/require-clean-work-tree-v3
Fetch-It-Via: git fetch https://github.com/dscho/git require-clean-work-tree-v3

Interdiff vs v2:

 diff --git a/builtin/pull.c b/builtin/pull.c
 index c639167..0bf9802 100644
 --- a/builtin/pull.c
 +++ b/builtin/pull.c
 @@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
  
  		if (!autostash)
  			require_clean_work_tree(N_("pull with rebase"),
 -				"Please commit or stash them.", 1, 0);
 +				"please commit or stash them.", 1, 0);
  
  		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
  			hashclr(rebase_fork_point);
 diff --git a/wt-status.c b/wt-status.c
 index a86918a..010276b 100644
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -2264,15 +2264,16 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub
  	rollback_lock_file(lock_file);
  
  	if (has_unstaged_changes(ignore_submodules)) {
 -		error(_("Cannot %s: You have unstaged changes."), _(action));
 +		/* TRANSLATORS: the action is e.g. "pull with rebase" */
 +		error(_("cannot %s: You have unstaged changes."), _(action));
  		err = 1;
  	}
  
  	if (has_uncommitted_changes(ignore_submodules)) {
  		if (err)
 -			error(_("Additionally, your index contains uncommitted changes."));
 +			error(_("additionally, your index contains uncommitted changes."));
  		else
 -			error(_("Cannot %s: Your index contains uncommitted changes."),
 +			error(_("cannot %s: Your index contains uncommitted changes."),
  			      _(action));
  		err = 1;
  	}

-- 
2.10.0.windows.1.325.ge6089c1

base-commit: 0cf36115dce7438a0eafad54a81cc57175e8fb54

[PATCH v3 1/6] pull: drop confusing prefix parameter of die_on_unclean_work_tree()

[Johannes Schindelin]

In cmd_pull(), when verifying that there are no changes preventing a
rebasing pull, we diligently pass the prefix parameter to the
die_on_unclean_work_tree() function which in turn diligently passes it
to the has_unstaged_changes() and has_uncommitted_changes() functions.

The casual reader might now be curious (as this developer was) whether
that means that calling `git pull --rebase` in a subdirectory will
ignore unstaged changes in other parts of the working directory. And be
puzzled that `git pull --rebase` (correctly) complains about those
changes outside of the current directory.

The puzzle is easily resolved: while we take pains to pass around the
prefix and even pass it to init_revisions(), the fact that no paths are
passed to init_revisions() ensures that the prefix is simply ignored.

That, combined with the fact that we will *always* want a *full* working
directory check before running a rebasing pull, is reason enough to
simply do away with the actual prefix parameter and to pass NULL
instead, as if we were running this from the top-level working directory
anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 398aae1..d4bd635 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -328,12 +328,12 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-static int has_unstaged_changes(const char *prefix)
+static int has_unstaged_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
 
-	init_revisions(&rev_info, prefix);
+	init_revisions(&rev_info, NULL);
 	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	diff_setup_done(&rev_info.diffopt);
@@ -344,7 +344,7 @@ static int has_unstaged_changes(const char *prefix)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-static int has_uncommitted_changes(const char *prefix)
+static int has_uncommitted_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
@@ -352,7 +352,7 @@ static int has_uncommitted_changes(const char *prefix)
 	if (is_cache_unborn())
 		return 0;
 
-	init_revisions(&rev_info, prefix);
+	init_revisions(&rev_info, NULL);
 	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	add_head_to_pending(&rev_info);
@@ -365,7 +365,7 @@ static int has_uncommitted_changes(const char *prefix)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-static void die_on_unclean_work_tree(const char *prefix)
+static void die_on_unclean_work_tree(void)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
 	int do_die = 0;
@@ -375,12 +375,12 @@ static void die_on_unclean_work_tree(const char *prefix)
 	update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
-	if (has_unstaged_changes(prefix)) {
+	if (has_unstaged_changes()) {
 		error(_("Cannot pull with rebase: You have unstaged changes."));
 		do_die = 1;
 	}
 
-	if (has_uncommitted_changes(prefix)) {
+	if (has_uncommitted_changes()) {
 		if (do_die)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
@@ -875,7 +875,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Updating an unborn branch with changes added to the index."));
 
 		if (!autostash)
-			die_on_unclean_work_tree(prefix);
+			die_on_unclean_work_tree();
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 2/6] pull: make code more similar to the shell script again

[Johannes Schindelin]

When converting the pull command to a builtin, the
require_clean_work_tree() function was renamed and the pull-specific
parts hard-coded.

This makes it impossible to reuse the code, so let's modify the code to
make it more similar to the original shell script again.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d4bd635..d1e093c 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -365,10 +365,11 @@ static int has_uncommitted_changes(void)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-static void die_on_unclean_work_tree(void)
+static int require_clean_work_tree(const char *action, const char *hint,
+		int gently)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-	int do_die = 0;
+	int err = 0;
 
 	hold_locked_index(lock_file, 0);
 	refresh_cache(REFRESH_QUIET);
@@ -376,20 +377,28 @@ static void die_on_unclean_work_tree(void)
 	rollback_lock_file(lock_file);
 
 	if (has_unstaged_changes()) {
-		error(_("Cannot pull with rebase: You have unstaged changes."));
-		do_die = 1;
+		/* TRANSLATORS: the action is e.g. "pull with rebase" */
+		error(_("Cannot %s: You have unstaged changes."), _(action));
+		err = 1;
 	}
 
 	if (has_uncommitted_changes()) {
-		if (do_die)
+		if (err)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
-			error(_("Cannot pull with rebase: Your index contains uncommitted changes."));
-		do_die = 1;
+			error(_("Cannot %s: Your index contains uncommitted changes."),
+			      _(action));
+		err = 1;
 	}
 
-	if (do_die)
-		exit(1);
+	if (err) {
+		if (hint)
+			error("%s", hint);
+		if (!gently)
+			exit(err);
+	}
+
+	return err;
 }
 
 /**
@@ -875,7 +884,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Updating an unborn branch with changes added to the index."));
 
 		if (!autostash)
-			die_on_unclean_work_tree();
+			require_clean_work_tree(N_("pull with rebase"),
+				"Please commit or stash them.", 0);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 3/6] Make the require_clean_work_tree() function reusable

[Johannes Schindelin]

It is remarkable that libgit.a did not sport this function yet... Let's
move it into a more prominent (and into an actually reusable) spot:
wt-status.[ch].

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 77 +---------------------------------------------------------
 wt-status.c    | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h    |  3 +++
 3 files changed, 80 insertions(+), 76 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d1e093c..14ef8b5 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -17,6 +17,7 @@
 #include "revision.h"
 #include "tempfile.h"
 #include "lockfile.h"
+#include "wt-status.h"
 
 enum rebase_type {
 	REBASE_INVALID = -1,
@@ -326,82 +327,6 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 }
 
 /**
- * Returns 1 if there are unstaged changes, 0 otherwise.
- */
-static int has_unstaged_changes(void)
-{
-	struct rev_info rev_info;
-	int result;
-
-	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_files(&rev_info, 0);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * Returns 1 if there are uncommitted changes, 0 otherwise.
- */
-static int has_uncommitted_changes(void)
-{
-	struct rev_info rev_info;
-	int result;
-
-	if (is_cache_unborn())
-		return 0;
-
-	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	add_head_to_pending(&rev_info);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_index(&rev_info, 1);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * If the work tree has unstaged or uncommitted changes, dies with the
- * appropriate message.
- */
-static int require_clean_work_tree(const char *action, const char *hint,
-		int gently)
-{
-	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-	int err = 0;
-
-	hold_locked_index(lock_file, 0);
-	refresh_cache(REFRESH_QUIET);
-	update_index_if_able(&the_index, lock_file);
-	rollback_lock_file(lock_file);
-
-	if (has_unstaged_changes()) {
-		/* TRANSLATORS: the action is e.g. "pull with rebase" */
-		error(_("Cannot %s: You have unstaged changes."), _(action));
-		err = 1;
-	}
-
-	if (has_uncommitted_changes()) {
-		if (err)
-			error(_("Additionally, your index contains uncommitted changes."));
-		else
-			error(_("Cannot %s: Your index contains uncommitted changes."),
-			      _(action));
-		err = 1;
-	}
-
-	if (err) {
-		if (hint)
-			error("%s", hint);
-		if (!gently)
-			exit(err);
-	}
-
-	return err;
-}
-
-/**
  * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
  * into merge_heads.
  */
diff --git a/wt-status.c b/wt-status.c
index 9628c1d..b92c54d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,7 @@
 #include "strbuf.h"
 #include "utf8.h"
 #include "worktree.h"
+#include "lockfile.h"
 
 static const char cut_line[] =
 "------------------------ >8 ------------------------\n";
@@ -2209,3 +2210,78 @@ void wt_status_print(struct wt_status *s)
 		break;
 	}
 }
+
+/**
+ * Returns 1 if there are unstaged changes, 0 otherwise.
+ */
+static int has_unstaged_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_files(&rev_info, 0);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * Returns 1 if there are uncommitted changes, 0 otherwise.
+ */
+static int has_uncommitted_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	if (is_cache_unborn())
+		return 0;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	add_head_to_pending(&rev_info);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_index(&rev_info, 1);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * If the work tree has unstaged or uncommitted changes, dies with the
+ * appropriate message.
+ */
+int require_clean_work_tree(const char *action, const char *hint, int gently)
+{
+	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
+	int err = 0;
+
+	hold_locked_index(lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	update_index_if_able(&the_index, lock_file);
+	rollback_lock_file(lock_file);
+
+	if (has_unstaged_changes()) {
+		/* TRANSLATORS: the action is e.g. "pull with rebase" */
+		error(_("Cannot %s: You have unstaged changes."), _(action));
+		err = 1;
+	}
+
+	if (has_uncommitted_changes()) {
+		if (err)
+			error(_("Additionally, your index contains uncommitted changes."));
+		else
+			error(_("Cannot %s: Your index contains uncommitted changes."),
+			      _(action));
+		err = 1;
+	}
+
+	if (err) {
+		if (hint)
+			error("%s", hint);
+		if (!gently)
+			exit(err);
+	}
+
+	return err;
+}
diff --git a/wt-status.h b/wt-status.h
index e401837..03ecf53 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,4 +128,7 @@ void status_printf_ln(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, ...);
 
+/* The following function expect that the caller took care of reading the index. */
+int require_clean_work_tree(const char *action, const char *hint, int gently);
+
 #endif /* STATUS_H */
-- 
2.10.0.windows.1.325.ge6089c1

Re: [PATCH v3 3/6] Make the require_clean_work_tree() function reusable

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> It is remarkable that libgit.a did not sport this function yet... Let's
> move it into a more prominent (and into an actually reusable) spot:
> wt-status.[ch].
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Thanks.

I'd tweak the message while queuing, though.

    wt-status: make the require_clean_work_tree() function reusable
    
    The function "git pull" uses to stop the user when the working
    tree has changes is useful in other places.
    
    Let's move it into a more prominent (and into an actually reusable)
    spot: wt-status.[ch].
    
    Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Readers won't care whether you found something remarkable and even
if they wanted to care, it is rather hard to sympathize with you
unless they know what "this function" does well enough to understand
why it is a good thing to make it available more widely.  That is a
more important point to address in the log message.

> diff --git a/wt-status.c b/wt-status.c
> index 9628c1d..b92c54d 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -16,6 +16,7 @@
>  #include "strbuf.h"
>  #include "utf8.h"
>  #include "worktree.h"
> +#include "lockfile.h"

Makes sense.  We'd be opportunistically refreshing the index.

Re: [PATCH v3 3/6] Make the require_clean_work_tree() function reusable

[Johannes Schindelin]

Hi Junio,

On Tue, 4 Oct 2016, Junio C Hamano wrote:

> I'd tweak the message while queuing, though.
> 
>     wt-status: make the require_clean_work_tree() function reusable
>     
>     The function "git pull" uses to stop the user when the working
>     tree has changes is useful in other places.

I stumbled over this sentence. How about

	The function used by "git pull" to stop [...]

instead?

Thanks,
Dscho

Re: [PATCH v3 3/6] Make the require_clean_work_tree() function reusable

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>>     The function "git pull" uses to stop the user when the working
>>     tree has changes is useful in other places.
>
> I stumbled over this sentence. How about
>
> 	The function used by "git pull" to stop [...]
>
> instead?

Perfect. Thanks.

[PATCH v3 4/6] Export also the has_un{staged,committed}_changed() functions

[Johannes Schindelin]

They will be used in the upcoming rebase helper.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 4 ++--
 wt-status.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index b92c54d..f4103e4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2214,7 +2214,7 @@ void wt_status_print(struct wt_status *s)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-static int has_unstaged_changes(void)
+int has_unstaged_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
@@ -2230,7 +2230,7 @@ static int has_unstaged_changes(void)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-static int has_uncommitted_changes(void)
+int has_uncommitted_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
diff --git a/wt-status.h b/wt-status.h
index 03ecf53..68e367a 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,7 +128,9 @@ void status_printf_ln(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, ...);
 
-/* The following function expect that the caller took care of reading the index. */
+/* The following functions expect that the caller took care of reading the index. */
+int has_unstaged_changes(void);
+int has_uncommitted_changes(void);
 int require_clean_work_tree(const char *action, const char *hint, int gently);
 
 #endif /* STATUS_H */
-- 
2.10.0.windows.1.325.ge6089c1

Re: [PATCH v3 4/6] Export also the has_un{staged,committed}_changed() functions

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> They will be used in the upcoming rebase helper.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

Makes sense.

Re: [PATCH v3 4/6] Export also the has_un{staged,committed}_changed() functions

[Jakub Narębski]

W dniu 04.10.2016 o 15:05, Johannes Schindelin pisze:

> Subject: Export also the has_un{staged,committed}_changed() functions

s/changed/changes/   that is   d -> s

Those are has_unstaged_changes() and has_uncommitted_changes().

Though I wonder if "other has_un*_changes() functions" would be
more readable (while shorter), if less specific...

>
> They will be used in the upcoming rebase helper.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
[...]
> -/* The following function expect that the caller took care of reading the index. */
> +/* The following functions expect that the caller took care of reading the index. */
> +int has_unstaged_changes(void);
> +int has_uncommitted_changes(void);
>  int require_clean_work_tree(const char *action, const char *hint, int gently);

Nice to see the fix in comment too.  Good work!

-- 
Jakub Narębski

[PATCH v3 5/6] wt-status: teach has_{unstaged,uncommitted}_changes() about submodules

[Johannes Schindelin]

Sometimes we are *actually* interested in those changes... For
example when an interactive rebase wants to continue with a staged
submodule update.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c |  2 +-
 wt-status.c    | 16 +++++++++-------
 wt-status.h    |  7 ++++---
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 14ef8b5..c639167 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 		if (!autostash)
 			require_clean_work_tree(N_("pull with rebase"),
-				"Please commit or stash them.", 0);
+				"Please commit or stash them.", 1, 0);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
diff --git a/wt-status.c b/wt-status.c
index f4103e4..061597a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2214,13 +2214,14 @@ void wt_status_print(struct wt_status *s)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-int has_unstaged_changes(void)
+int has_unstaged_changes(int ignore_submodules)
 {
 	struct rev_info rev_info;
 	int result;
 
 	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	if (ignore_submodules)
+		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	diff_setup_done(&rev_info.diffopt);
 	result = run_diff_files(&rev_info, 0);
@@ -2230,7 +2231,7 @@ int has_unstaged_changes(void)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-int has_uncommitted_changes(void)
+int has_uncommitted_changes(int ignore_submodules)
 {
 	struct rev_info rev_info;
 	int result;
@@ -2239,7 +2240,8 @@ int has_uncommitted_changes(void)
 		return 0;
 
 	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	if (ignore_submodules)
+		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	add_head_to_pending(&rev_info);
 	diff_setup_done(&rev_info.diffopt);
@@ -2251,7 +2253,7 @@ int has_uncommitted_changes(void)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-int require_clean_work_tree(const char *action, const char *hint, int gently)
+int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
 	int err = 0;
@@ -2261,13 +2263,13 @@ int require_clean_work_tree(const char *action, const char *hint, int gently)
 	update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
-	if (has_unstaged_changes()) {
+	if (has_unstaged_changes(ignore_submodules)) {
 		/* TRANSLATORS: the action is e.g. "pull with rebase" */
 		error(_("Cannot %s: You have unstaged changes."), _(action));
 		err = 1;
 	}
 
-	if (has_uncommitted_changes()) {
+	if (has_uncommitted_changes(ignore_submodules)) {
 		if (err)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
diff --git a/wt-status.h b/wt-status.h
index 68e367a..54fec77 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -129,8 +129,9 @@ __attribute__((format (printf, 3, 4)))
 void status_printf(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(void);
-int has_uncommitted_changes(void);
-int require_clean_work_tree(const char *action, const char *hint, int gently);
+int has_unstaged_changes(int ignore_submodules);
+int has_uncommitted_changes(int ignore_submodules);
+int require_clean_work_tree(const char *action, const char *hint,
+	int ignore_submodules, int gently);
 
 #endif /* STATUS_H */
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v3 6/6] wt-status: begin error messages with lower-case

[Johannes Schindelin]

The previous code still followed the old git-pull.sh code which did not
adhere to our new convention.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 2 +-
 wt-status.c    | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index c639167..0bf9802 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 		if (!autostash)
 			require_clean_work_tree(N_("pull with rebase"),
-				"Please commit or stash them.", 1, 0);
+				"please commit or stash them.", 1, 0);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
diff --git a/wt-status.c b/wt-status.c
index 061597a..010276b 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2265,15 +2265,15 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub
 
 	if (has_unstaged_changes(ignore_submodules)) {
 		/* TRANSLATORS: the action is e.g. "pull with rebase" */
-		error(_("Cannot %s: You have unstaged changes."), _(action));
+		error(_("cannot %s: You have unstaged changes."), _(action));
 		err = 1;
 	}
 
 	if (has_uncommitted_changes(ignore_submodules)) {
 		if (err)
-			error(_("Additionally, your index contains uncommitted changes."));
+			error(_("additionally, your index contains uncommitted changes."));
 		else
-			error(_("Cannot %s: Your index contains uncommitted changes."),
+			error(_("cannot %s: Your index contains uncommitted changes."),
 			      _(action));
 		err = 1;
 	}
-- 
2.10.0.windows.1.325.ge6089c1

Re: [PATCH v3 6/6] wt-status: begin error messages with lower-case

[Jakub Narębski]

W dniu 04.10.2016 o 15:06, Johannes Schindelin pisze:

> The previous code still followed the old git-pull.sh code which did not
> adhere to our new convention.

Good to know why it used its own convention.
 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  builtin/pull.c | 2 +-
>  wt-status.c    | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/builtin/pull.c b/builtin/pull.c
> index c639167..0bf9802 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>  
>  		if (!autostash)
>  			require_clean_work_tree(N_("pull with rebase"),
> -				"Please commit or stash them.", 1, 0);
> +				"please commit or stash them.", 1, 0);
>  

Shouldn't those also be marked for translation with N_() or _()?

Best,
-- 
Jakub Narębski

Re: [PATCH v3 6/6] wt-status: begin error messages with lower-case

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

Hi Kuba,

On Wed, 5 Oct 2016, Jakub Narębski wrote:

> W dniu 04.10.2016 o 15:06, Johannes Schindelin pisze:
> 
> > The previous code still followed the old git-pull.sh code which did not
> > adhere to our new convention.
> 
> Good to know why it used its own convention.

Yeah, I figured that it is late, but still a good thing to explain this...

> > diff --git a/builtin/pull.c b/builtin/pull.c
> > index c639167..0bf9802 100644
> > --- a/builtin/pull.c
> > +++ b/builtin/pull.c
> > @@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> >  
> >  		if (!autostash)
> >  			require_clean_work_tree(N_("pull with rebase"),
> > -				"Please commit or stash them.", 1, 0);
> > +				"please commit or stash them.", 1, 0);
> >  
> 
> Shouldn't those also be marked for translation with N_() or _()?

Of course!

Thanks,
Dscho

Re: [PATCH v3 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> This is the 5th last patch series of my work to accelerate interactive
> rebases in particular on Windows.

Offtopic, but I am always confused by what you might mean by this
"nth last patch series".  Is this series 5th from the last and we
have four more to go?

In any case, after a quick re-read and comparison with the last
round, I think this is in a good shape.  I'd say that we would wait
for a few days for others to comment and then merge it to 'next' if
we missed nothing glaringly wrong.

Thanks.

Re: [PATCH v3 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c

[Johannes Schindelin]

Hi Junio,

On Tue, 4 Oct 2016, Junio C Hamano wrote:

> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> > This is the 5th last patch series of my work to accelerate interactive
> > rebases in particular on Windows.
> 
> Offtopic, but I am always confused by what you might mean by this
> "nth last patch series".  Is this series 5th from the last and we
> have four more to go?

Yes, we have four more to go:

- the patch series (called "prepare-sequencer" in my fork) preparing the
  sequencer code for the next patch series, such as revamping the parser
  for the edit script (or "todo script" as I used to say all the time, or
  "insn sheet" as sequencer calls it),

- the patch series ("sequencer-i") teaching the sequencer to parse and
  execute the commands of rebase -i's git-rebase-todo file,

- the patch series ("rebase--helper") introducing the builtin, and using
  it from rebase -i, and finally

- the patch series ("rebase-i-extra") that moves more performance critical
  bits and pieces from git-rebase--interactive.sh into the rebase--helper.

I had originally planned to stop at rebase--helper and invite other
developers to join the fun of making rebase -i a true builtin, but the
performance improvement was surprisingly disappointing before the
rebase--helper learned to skip unnecessary picks, to verify that the
script is valid, to expand/collapse the SHA-1s, and to rearrange
fixup!/squash!  lines.

> In any case, after a quick re-read and comparison with the last
> round, I think this is in a good shape.  I'd say that we would wait
> for a few days for others to comment and then merge it to 'next' if
> we missed nothing glaringly wrong.

Perfect!
Dscho

[PATCH v4 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c

[Johannes Schindelin]

This is the 5th last patch series of my work to accelerate interactive
rebases in particular on Windows.

Basically, all it does is to make reusable some functions that were
ported over from git-pull.sh but made private to builtin/pull.c.

Changes since v3:

- reworded 3/5's commit message according to Junio's suggestion.

- fixed a tyop in 4/5's commit message, pointed out by Jakub.

- marked the hint "please commit or stash them" (reintroduced from the
  original git-pull.sh script) as translatable.

- changed the exit code to 128 (emulating a die()) if
  require_clean_work-tree() was asked to be non-gentle.

- fixed a tyop in 3/6 (which was replaced in 4/6, but it is good not to
  introduce bugs only to fix them right away).

- prefixed the commit message of 4/6 with the "wt-status:" prefix,
  replicating Junio's commit message in the `pu` branch.


Johannes Schindelin (6):
  pull: drop confusing prefix parameter of die_on_unclean_work_tree()
  pull: make code more similar to the shell script again
  wt-status: make the require_clean_work_tree() function reusable
  wt-status: export also the has_un{staged,committed}_changes()
    functions
  wt-status: teach has_{unstaged,uncommitted}_changes() about submodules
  wt-status: begin error messages with lower-case

 builtin/pull.c | 71 +++-------------------------------------------------
 wt-status.c    | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h    |  6 +++++
 3 files changed, 87 insertions(+), 68 deletions(-)

Published-As: https://github.com/dscho/git/releases/tag/require-clean-work-tree-v4
Fetch-It-Via: git fetch https://github.com/dscho/git require-clean-work-tree-v4

Interdiff vs v3:

 diff --git a/builtin/pull.c b/builtin/pull.c
 index 0bf9802..d6e46ee 100644
 --- a/builtin/pull.c
 +++ b/builtin/pull.c
 @@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
  
  		if (!autostash)
  			require_clean_work_tree(N_("pull with rebase"),
 -				"please commit or stash them.", 1, 0);
 +				_("please commit or stash them."), 1, 0);
  
  		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
  			hashclr(rebase_fork_point);
 diff --git a/wt-status.c b/wt-status.c
 index ef67593..e8e5de4 100644
 --- a/wt-status.c
 +++ b/wt-status.c
 @@ -2281,7 +2281,7 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub
  		if (hint)
  			error("%s", hint);
  		if (!gently)
 -			exit(err);
 +			exit(128);
  	}
  
  	return err;

-- 
2.10.0.windows.1.325.ge6089c1

base-commit: a23ca1b8dc42ffd4de2ef30d67ce1e21ded29886

[PATCH v4 1/6] pull: drop confusing prefix parameter of die_on_unclean_work_tree()

[Johannes Schindelin]

In cmd_pull(), when verifying that there are no changes preventing a
rebasing pull, we diligently pass the prefix parameter to the
die_on_unclean_work_tree() function which in turn diligently passes it
to the has_unstaged_changes() and has_uncommitted_changes() functions.

The casual reader might now be curious (as this developer was) whether
that means that calling `git pull --rebase` in a subdirectory will
ignore unstaged changes in other parts of the working directory. And be
puzzled that `git pull --rebase` (correctly) complains about those
changes outside of the current directory.

The puzzle is easily resolved: while we take pains to pass around the
prefix and even pass it to init_revisions(), the fact that no paths are
passed to init_revisions() ensures that the prefix is simply ignored.

That, combined with the fact that we will *always* want a *full* working
directory check before running a rebasing pull, is reason enough to
simply do away with the actual prefix parameter and to pass NULL
instead, as if we were running this from the top-level working directory
anyway.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 398aae1..d4bd635 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -328,12 +328,12 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-static int has_unstaged_changes(const char *prefix)
+static int has_unstaged_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
 
-	init_revisions(&rev_info, prefix);
+	init_revisions(&rev_info, NULL);
 	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	diff_setup_done(&rev_info.diffopt);
@@ -344,7 +344,7 @@ static int has_unstaged_changes(const char *prefix)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-static int has_uncommitted_changes(const char *prefix)
+static int has_uncommitted_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
@@ -352,7 +352,7 @@ static int has_uncommitted_changes(const char *prefix)
 	if (is_cache_unborn())
 		return 0;
 
-	init_revisions(&rev_info, prefix);
+	init_revisions(&rev_info, NULL);
 	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	add_head_to_pending(&rev_info);
@@ -365,7 +365,7 @@ static int has_uncommitted_changes(const char *prefix)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-static void die_on_unclean_work_tree(const char *prefix)
+static void die_on_unclean_work_tree(void)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
 	int do_die = 0;
@@ -375,12 +375,12 @@ static void die_on_unclean_work_tree(const char *prefix)
 	update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
-	if (has_unstaged_changes(prefix)) {
+	if (has_unstaged_changes()) {
 		error(_("Cannot pull with rebase: You have unstaged changes."));
 		do_die = 1;
 	}
 
-	if (has_uncommitted_changes(prefix)) {
+	if (has_uncommitted_changes()) {
 		if (do_die)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
@@ -875,7 +875,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Updating an unborn branch with changes added to the index."));
 
 		if (!autostash)
-			die_on_unclean_work_tree(prefix);
+			die_on_unclean_work_tree();
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v4 2/6] pull: make code more similar to the shell script again

[Johannes Schindelin]

When converting the pull command to a builtin, the
require_clean_work_tree() function was renamed and the pull-specific
parts hard-coded.

This makes it impossible to reuse the code, so let's modify the code to
make it more similar to the original shell script again.

Note: when the hint "Please commit or stash them" was introduced first,
Git did not have the convention of continuing error messages in lower
case, but now we do have that convention, therefore we reintroduce this
hint down-cased, obeying said convention.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index d4bd635..58fc176 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -365,10 +365,11 @@ static int has_uncommitted_changes(void)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-static void die_on_unclean_work_tree(void)
+static int require_clean_work_tree(const char *action, const char *hint,
+		int gently)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-	int do_die = 0;
+	int err = 0;
 
 	hold_locked_index(lock_file, 0);
 	refresh_cache(REFRESH_QUIET);
@@ -376,20 +377,28 @@ static void die_on_unclean_work_tree(void)
 	rollback_lock_file(lock_file);
 
 	if (has_unstaged_changes()) {
-		error(_("Cannot pull with rebase: You have unstaged changes."));
-		do_die = 1;
+		/* TRANSLATORS: the action is e.g. "pull with rebase" */
+		error(_("Cannot %s: You have unstaged changes."), _(action));
+		err = 1;
 	}
 
 	if (has_uncommitted_changes()) {
-		if (do_die)
+		if (err)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
-			error(_("Cannot pull with rebase: Your index contains uncommitted changes."));
-		do_die = 1;
+			error(_("Cannot %s: Your index contains uncommitted changes."),
+			      _(action));
+		err = 1;
 	}
 
-	if (do_die)
-		exit(1);
+	if (err) {
+		if (hint)
+			error("%s", hint);
+		if (!gently)
+			exit(128);
+	}
+
+	return err;
 }
 
 /**
@@ -875,7 +884,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			die(_("Updating an unborn branch with changes added to the index."));
 
 		if (!autostash)
-			die_on_unclean_work_tree();
+			require_clean_work_tree(N_("pull with rebase"),
+				_("please commit or stash them."), 0);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v4 3/6] wt-status: make the require_clean_work_tree() function reusable

[Johannes Schindelin]

The function used by "git pull" to stop the user when the working
tree has changes is useful in other places.

Let's move it into a more prominent (and into an actually reusable)
spot: wt-status.[ch].

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c | 77 +---------------------------------------------------------
 wt-status.c    | 76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h    |  3 +++
 3 files changed, 80 insertions(+), 76 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 58fc176..01b6465 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -17,6 +17,7 @@
 #include "revision.h"
 #include "tempfile.h"
 #include "lockfile.h"
+#include "wt-status.h"
 
 enum rebase_type {
 	REBASE_INVALID = -1,
@@ -326,82 +327,6 @@ static int git_pull_config(const char *var, const char *value, void *cb)
 }
 
 /**
- * Returns 1 if there are unstaged changes, 0 otherwise.
- */
-static int has_unstaged_changes(void)
-{
-	struct rev_info rev_info;
-	int result;
-
-	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_files(&rev_info, 0);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * Returns 1 if there are uncommitted changes, 0 otherwise.
- */
-static int has_uncommitted_changes(void)
-{
-	struct rev_info rev_info;
-	int result;
-
-	if (is_cache_unborn())
-		return 0;
-
-	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	add_head_to_pending(&rev_info);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_index(&rev_info, 1);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
- * If the work tree has unstaged or uncommitted changes, dies with the
- * appropriate message.
- */
-static int require_clean_work_tree(const char *action, const char *hint,
-		int gently)
-{
-	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
-	int err = 0;
-
-	hold_locked_index(lock_file, 0);
-	refresh_cache(REFRESH_QUIET);
-	update_index_if_able(&the_index, lock_file);
-	rollback_lock_file(lock_file);
-
-	if (has_unstaged_changes()) {
-		/* TRANSLATORS: the action is e.g. "pull with rebase" */
-		error(_("Cannot %s: You have unstaged changes."), _(action));
-		err = 1;
-	}
-
-	if (has_uncommitted_changes()) {
-		if (err)
-			error(_("Additionally, your index contains uncommitted changes."));
-		else
-			error(_("Cannot %s: Your index contains uncommitted changes."),
-			      _(action));
-		err = 1;
-	}
-
-	if (err) {
-		if (hint)
-			error("%s", hint);
-		if (!gently)
-			exit(128);
-	}
-
-	return err;
-}
-
-/**
  * Appends merge candidates from FETCH_HEAD that are not marked not-for-merge
  * into merge_heads.
  */
diff --git a/wt-status.c b/wt-status.c
index 99d1b0a..89475f1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,7 @@
 #include "strbuf.h"
 #include "utf8.h"
 #include "worktree.h"
+#include "lockfile.h"
 
 static const char cut_line[] =
 "------------------------ >8 ------------------------\n";
@@ -2208,3 +2209,78 @@ void wt_status_print(struct wt_status *s)
 		break;
 	}
 }
+
+/**
+ * Returns 1 if there are unstaged changes, 0 otherwise.
+ */
+static int has_unstaged_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_files(&rev_info, 0);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * Returns 1 if there are uncommitted changes, 0 otherwise.
+ */
+static int has_uncommitted_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	if (is_cache_unborn())
+		return 0;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	add_head_to_pending(&rev_info);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_index(&rev_info, 1);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
+/**
+ * If the work tree has unstaged or uncommitted changes, dies with the
+ * appropriate message.
+ */
+int require_clean_work_tree(const char *action, const char *hint, int gently)
+{
+	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
+	int err = 0;
+
+	hold_locked_index(lock_file, 0);
+	refresh_cache(REFRESH_QUIET);
+	update_index_if_able(&the_index, lock_file);
+	rollback_lock_file(lock_file);
+
+	if (has_unstaged_changes()) {
+		/* TRANSLATORS: the action is e.g. "pull with rebase" */
+		error(_("Cannot %s: You have unstaged changes."), _(action));
+		err = 1;
+	}
+
+	if (has_uncommitted_changes()) {
+		if (err)
+			error(_("Additionally, your index contains uncommitted changes."));
+		else
+			error(_("Cannot %s: Your index contains uncommitted changes."),
+			      _(action));
+		err = 1;
+	}
+
+	if (err) {
+		if (hint)
+			error("%s", hint);
+		if (!gently)
+			exit(128);
+	}
+
+	return err;
+}
diff --git a/wt-status.h b/wt-status.h
index e401837..68b4709 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,4 +128,7 @@ void status_printf_ln(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, ...);
 
+/* The following function expects that the caller took care of reading the index. */
+int require_clean_work_tree(const char *action, const char *hint, int gently);
+
 #endif /* STATUS_H */
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v4 4/6] wt-status: export also the has_un{staged,committed}_changes() functions

[Johannes Schindelin]

They will be used in the upcoming rebase helper.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 4 ++--
 wt-status.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 89475f1..8fe7d0a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2213,7 +2213,7 @@ void wt_status_print(struct wt_status *s)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-static int has_unstaged_changes(void)
+int has_unstaged_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
@@ -2229,7 +2229,7 @@ static int has_unstaged_changes(void)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-static int has_uncommitted_changes(void)
+int has_uncommitted_changes(void)
 {
 	struct rev_info rev_info;
 	int result;
diff --git a/wt-status.h b/wt-status.h
index 68b4709..68e367a 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,7 +128,9 @@ void status_printf_ln(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, ...);
 
-/* The following function expects that the caller took care of reading the index. */
+/* The following functions expect that the caller took care of reading the index. */
+int has_unstaged_changes(void);
+int has_uncommitted_changes(void);
 int require_clean_work_tree(const char *action, const char *hint, int gently);
 
 #endif /* STATUS_H */
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v4 5/6] wt-status: teach has_{unstaged,uncommitted}_changes() about submodules

[Johannes Schindelin]

Sometimes we are *actually* interested in those changes... For
example when an interactive rebase wants to continue with a staged
submodule update.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin/pull.c |  2 +-
 wt-status.c    | 16 +++++++++-------
 wt-status.h    |  7 ++++---
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 01b6465..d6e46ee 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -810,7 +810,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 		if (!autostash)
 			require_clean_work_tree(N_("pull with rebase"),
-				_("please commit or stash them."), 0);
+				_("please commit or stash them."), 1, 0);
 
 		if (get_rebase_fork_point(rebase_fork_point, repo, *refspecs))
 			hashclr(rebase_fork_point);
diff --git a/wt-status.c b/wt-status.c
index 8fe7d0a..0020140 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2213,13 +2213,14 @@ void wt_status_print(struct wt_status *s)
 /**
  * Returns 1 if there are unstaged changes, 0 otherwise.
  */
-int has_unstaged_changes(void)
+int has_unstaged_changes(int ignore_submodules)
 {
 	struct rev_info rev_info;
 	int result;
 
 	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	if (ignore_submodules)
+		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	diff_setup_done(&rev_info.diffopt);
 	result = run_diff_files(&rev_info, 0);
@@ -2229,7 +2230,7 @@ int has_unstaged_changes(void)
 /**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
-int has_uncommitted_changes(void)
+int has_uncommitted_changes(int ignore_submodules)
 {
 	struct rev_info rev_info;
 	int result;
@@ -2238,7 +2239,8 @@ int has_uncommitted_changes(void)
 		return 0;
 
 	init_revisions(&rev_info, NULL);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	if (ignore_submodules)
+		DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
 	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
 	add_head_to_pending(&rev_info);
 	diff_setup_done(&rev_info.diffopt);
@@ -2250,7 +2252,7 @@ int has_uncommitted_changes(void)
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
-int require_clean_work_tree(const char *action, const char *hint, int gently)
+int require_clean_work_tree(const char *action, const char *hint, int ignore_submodules, int gently)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(*lock_file));
 	int err = 0;
@@ -2260,13 +2262,13 @@ int require_clean_work_tree(const char *action, const char *hint, int gently)
 	update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
-	if (has_unstaged_changes()) {
+	if (has_unstaged_changes(ignore_submodules)) {
 		/* TRANSLATORS: the action is e.g. "pull with rebase" */
 		error(_("Cannot %s: You have unstaged changes."), _(action));
 		err = 1;
 	}
 
-	if (has_uncommitted_changes()) {
+	if (has_uncommitted_changes(ignore_submodules)) {
 		if (err)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
diff --git a/wt-status.h b/wt-status.h
index 68e367a..54fec77 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -129,8 +129,9 @@ __attribute__((format (printf, 3, 4)))
 void status_printf(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(void);
-int has_uncommitted_changes(void);
-int require_clean_work_tree(const char *action, const char *hint, int gently);
+int has_unstaged_changes(int ignore_submodules);
+int has_uncommitted_changes(int ignore_submodules);
+int require_clean_work_tree(const char *action, const char *hint,
+	int ignore_submodules, int gently);
 
 #endif /* STATUS_H */
-- 
2.10.0.windows.1.325.ge6089c1

[PATCH v4 6/6] wt-status: begin error messages with lower-case

[Johannes Schindelin]

The previous code still followed the old git-pull.sh code which did not
adhere to our new convention.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 wt-status.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 0020140..e8e5de4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -2264,15 +2264,15 @@ int require_clean_work_tree(const char *action, const char *hint, int ignore_sub
 
 	if (has_unstaged_changes(ignore_submodules)) {
 		/* TRANSLATORS: the action is e.g. "pull with rebase" */
-		error(_("Cannot %s: You have unstaged changes."), _(action));
+		error(_("cannot %s: You have unstaged changes."), _(action));
 		err = 1;
 	}
 
 	if (has_uncommitted_changes(ignore_submodules)) {
 		if (err)
-			error(_("Additionally, your index contains uncommitted changes."));
+			error(_("additionally, your index contains uncommitted changes."));
 		else
-			error(_("Cannot %s: Your index contains uncommitted changes."),
+			error(_("cannot %s: Your index contains uncommitted changes."),
 			      _(action));
 		err = 1;
 	}
-- 
2.10.0.windows.1.325.ge6089c1

Re: [PATCH v4 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> - changed the exit code to 128 (emulating a die()) if
>   require_clean_work-tree() was asked to be non-gentle.

Ah, I didn't spot this the last round.  Good change.

Thanks, will replace.

Re: [PATCH v4 0/6] Pull out require_clean_work_tree() functionality from builtin/pull.c

[Jakub Narębski]

W dniu 07.10.2016 o 18:08, Johannes Schindelin pisze:

> - marked the hint "please commit or stash them" (reintroduced from the
>   original git-pull.sh script) as translatable.

I wonder if we can make automatic check if everything introduced is
translatable, for example with something akin to "English (XT)"
autogenerated pseudo-localization that Android uses?

Just food for thought.
-- 
Jakub Narębski