[PATCH 3/5] diff: remove unused #include "sigchain.h"

2015-10-22 Thread Tobias Klauser 
After switching to use the tempfile module in commit 284098f1
(diff: use tempfile module), no declarations from sigchain.h are used in
diff.c anymore. Thus, remove the #include.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 diff.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/diff.c b/diff.c
index 46260ed..df47592 100644
--- a/diff.c
+++ b/diff.c
@@ -13,7 +13,6 @@
 #include "run-command.h"
 #include "utf8.h"
 #include "userdiff.h"
-#include "sigchain.h"
 #include "submodule-config.h"
 #include "submodule.h"
 #include "ll-merge.h"
-- 
2.6.1.148.g7927db1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] credential-cache--daemon: remove unused #include "sigchain.h"

2015-10-22 Thread Tobias Klauser 
After switching to use the tempfile module in commit 9e903316
(credential-cache--daemon: use tempfile module), no declarations from
sigchain.h are used in credential-cache--daemon.c anymore. Thus, remove
the #include.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 credential-cache--daemon.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index eef6fce..82715aa 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -2,7 +2,6 @@
 #include "tempfile.h"
 #include "credential.h"
 #include "unix-socket.h"
-#include "sigchain.h"
 #include "parse-options.h"
 
 static struct tempfile socket_file;
-- 
2.6.1.148.g7927db1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] gc: remove unused #include "sigchain.h"

2015-10-22 Thread Tobias Klauser 
After switching to use the tempfile module in commit ebebeaea (gc: use
tempfile module to handle gc.pid file), no declarations from sigchain.h
are used in builtin/gc.c anymore. Thus, remove the #include.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 builtin/gc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 9ff0204..dc8a242 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -15,7 +15,6 @@
 #include "lockfile.h"
 #include "parse-options.h"
 #include "run-command.h"
-#include "sigchain.h"
 #include "argv-array.h"
 #include "commit.h"
 
-- 
2.6.1.148.g7927db1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] shallow: remove unused #include "sigchain.h"

2015-10-22 Thread Tobias Klauser 
After switching to use the tempfile module in commit 6e122b44
(setup_temporary_shallow(): use tempfile module), no declarations from
sigchain.h are used in read-cache.c anymore. Thus, remove the #include.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 shallow.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/shallow.c b/shallow.c
index d49a3d6..4f9d667 100644
--- a/shallow.c
+++ b/shallow.c
@@ -10,7 +10,6 @@
 #include "diff.h"
 #include "revision.h"
 #include "commit-slab.h"
-#include "sigchain.h"
 
 static int is_shallow = -1;
 static struct stat_validity shallow_stat;
-- 
2.6.1.148.g7927db1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/5] read-cache: remove unused #include "sigchain.h"

2015-10-22 Thread Tobias Klauser 
After switching to use the tempfile module in commit f6ecc62d
(write_shared_index(): use tempfile module), no declarations from
sigchain.h are used in read-cache.c anymore. Thus, remove the #include.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 read-cache.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/read-cache.c b/read-cache.c
index 87204a5..3ecb99d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -17,7 +17,6 @@
 #include "strbuf.h"
 #include "varint.h"
 #include "split-index.h"
-#include "sigchain.h"
 #include "utf8.h"
 
 static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
-- 
2.6.1.148.g7927db1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/5] Remove unused #include "sigchain.h"

2015-10-22 Thread Tobias Klauser 
This series removes the #include of sigchain.h from several modules
after they were changed to use the tempfile module and they thus no
longer use any declarations from sigchain.h

Tobias Klauser (5):
  gc: remove unused #include "sigchain.h"
  credential-cache--daemon: remove unused #include "sigchain.h"
  diff: remove unused #include "sigchain.h"
  read-cache: remove unused #include "sigchain.h"
  shallow: remove unused #include "sigchain.h"

 builtin/gc.c   | 1 -
 credential-cache--daemon.c | 1 -
 diff.c | 1 -
 read-cache.c   | 1 -
 shallow.c  | 1 -
 5 files changed, 5 deletions(-)

-- 
2.6.1.148.g7927db1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] gc: remove unused #include "sigchain.h"

2015-10-23 Thread Tobias Klauser 
On 2015-10-22 at 22:14:29 +0200, Junio C Hamano <gits...@pobox.com> wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> > After switching to use the tempfile module in commit ebebeaea (gc: use
> > tempfile module to handle gc.pid file), no declarations from sigchain.h
> > are used in builtin/gc.c anymore. Thus, remove the #include.
> >
> > Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
> > ---
> 
> I'll drop this one as we seem to have gained another user of the API
> in this file in the nd/gc-auto-background-fix topic.

Opps, sorry I didn't check against 'pu' carefully enough. Thanks for
catching it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

2015-10-19 Thread Tobias Klauser 
On 2015-10-18 at 19:18:53 +0200, Junio C Hamano  wrote:
> Eric Sunshine  writes:
> 
> > Is there any application beyond git-rebase--interactive where a
> > --count-lines options is expected to be useful? It's not obvious from
> > the commit message that this change is necessarily a win for later
> > porting of git-rebase--interactive to C since the amount of extra code
> > and support material added by this patch probably outweighs the amount
> > of code a C version of git-rebase--interactive would need to count the
> > lines itself.
> >
> > Stated differently, are the two or three instances of piping through
> > 'wc' in git-rebase--interactive sufficient justification for
> > introducing extra complexity into git-stripspace and its documentation
> > and tests?
> 
> Interesting thought.  When somebody rewrites "rebase -i" in C,
> nobody needs to count lines in "stripspace" output.  The rewritten
> "rebase -i" would internally run strbuf_stripspace() and the question
> becomes what is the best way to let that code find out how many lines
> the result contains.
> 
> When viewed from that angle, I agree that "stripspace --count" does
> not add anything to further the goal of helping "rebase -i" to move
> to C.  Adding strbuf_count_lines() that counts the number of lines
> in the given strbuf (if there is no such helper yet; I didn't check),
> though.

I check before implementing this series and didn't find any helper. I
also didn't find any other uses of line counting in the code.

After considering your and Eric's reply, I'll drop these patches for
now and only resubmit patches 1/4 and 2/4 for v3 (also see my reply to
Eric).

> >> +test_expect_success '--count-lines with newline only' '
> >> +   printf "0\n" >expect &&
> >> +   printf "\n" | git stripspace --count-lines >actual &&
> >> +   test_cmp expect actual
> >> +'
> >
> > What is the expected behavior when the input is an empty file, a file
> > with content but no newline, a file with one or more lines but lacking
> > a newline on the final line? Should these cases be tested, as well?
> 
> Good point here, too.  If we were to add strbuf_count_lines()
> helper, whoever adds that function needs to take a possible
> incomplete line at the end into account.

Yes, makes more sense like this (even though it doesn't correspond to
what 'wc -l' does).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/4] stripspace: Implement --count-lines option

2015-10-19 Thread Tobias Klauser 
On 2015-10-18 at 01:57:57 +0200, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Fri, Oct 16, 2015 at 11:16 AM, Tobias Klauser <tklau...@distanz.ch> wrote:
> > Implement the --count-lines options for git stripspace [...]
> >
> > This will make it easier to port git-rebase--interactive.sh to C later
> > on.
> 
> Is there any application beyond git-rebase--interactive where a
> --count-lines options is expected to be useful? It's not obvious from
> the commit message that this change is necessarily a win for later
> porting of git-rebase--interactive to C since the amount of extra code
> and support material added by this patch probably outweighs the amount
> of code a C version of git-rebase--interactive would need to count the
> lines itself.

Agreed, it doesn't make much sense anymore in the current form. An
strbuf helper function implementing the line counting would probably be
the better way. But I guess this should only be introduced once someone
decides to write a C version of git-rebase--interactive (or any other
use for line counting appears).

> Stated differently, are the two or three instances of piping through
> 'wc' in git-rebase--interactive sufficient justification for
> introducing extra complexity into git-stripspace and its documentation
> and tests?

IMO it doesn't add a lot of complexity, but on the other hand it also
doesn't provide a large benefit apart from getting rid of a few
calls to an external program in a code path which is not performance
critical.

So I suggest I'll drop patches 3/4 and 4/4 for v3. Once someone really
needs the line counting functionality in C, an strbuf helper can still
be added.

> More below.
> 
> > Furthermore, add the corresponding documentation and tests.
> >
> > Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
> > ---
> > diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
> > index 29e91d8..9c00cb9 100755
> > --- a/t/t0030-stripspace.sh
> > +++ b/t/t0030-stripspace.sh
> > @@ -438,4 +438,40 @@ test_expect_success 'avoid SP-HT sequence in commented 
> > line' '
> > test_cmp expect actual
> >  '
> >
> > +test_expect_success '--count-lines with newline only' '
> > +   printf "0\n" >expect &&
> > +   printf "\n" | git stripspace --count-lines >actual &&
> > +   test_cmp expect actual
> > +'
> 
> What is the expected behavior when the input is an empty file, a file
> with content but no newline, a file with one or more lines but lacking
> a newline on the final line? Should these cases be tested, as well?

Not really sure. For the implementation I followed the behavior of 'wc
-l' which doesn't consider the final line if it lacks a newline. Should
this be different for git's purposes? In any case, I agree that these
cases should excplicitely be tested/documented.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] stripspace: Implement and use --count-lines option

2015-10-15 Thread Tobias Klauser 
This patch set implements some of the project ideas around git stripspace
suggested on [1].

[1] https://git.wiki.kernel.org/index.php/SmallProjectsIdeas

The first patch moves the stripspace() function to the
strbuf module (adding a prefix and changing all users accordingly, also
a wrapper is introduced in case any topic branches still depend on the
old name). The second patch introduces option --count-lines to git
stripspace and also adds documentation and tests accordingly, Finally,
the third patch changes git-rebase--interactive.sh to replace commands
like:

git stripspace ... | wc -l

with:

git stripspace --count-lines ...

Tobias Klauser (3):
  strbuf: make stripspace() part of strbuf
  stripspace: Implement --count-lines option
  git rebase -i: Use newly added --count-lines option for stripspace

 Documentation/git-stripspace.txt |  13 -
 builtin/am.c |   2 +-
 builtin/branch.c |   2 +-
 builtin/commit.c |   6 +-
 builtin/merge.c  |   2 +-
 builtin/notes.c  |   6 +-
 builtin/stripspace.c | 122 ++-
 builtin/tag.c|   2 +-
 git-rebase--interactive.sh   |   6 +-
 strbuf.c |  72 +++
 strbuf.h |  14 -
 t/t0030-stripspace.sh|  36 
 12 files changed, 177 insertions(+), 106 deletions(-)

-- 
2.6.1.145.gb27dacc


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] strbuf: make stripspace() part of strbuf

2015-10-15 Thread Tobias Klauser 
Rename stripspace() to strbuf_stripspace() and move it to the strbuf
module as suggested in [1].

Also switch all current users of stripspace() to the new function name
and  keep a temporary wrapper inline function for topic branches still
using stripspace().

[1] 
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 builtin/am.c |  2 +-
 builtin/branch.c |  2 +-
 builtin/commit.c |  6 ++---
 builtin/merge.c  |  2 +-
 builtin/notes.c  |  6 ++---
 builtin/stripspace.c | 69 ++--
 builtin/tag.c|  2 +-
 strbuf.c | 66 +
 strbuf.h | 11 -
 9 files changed, 88 insertions(+), 78 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 4f77e07..fbe9152 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1343,7 +1343,7 @@ static int parse_mail(struct am_state *state, const char 
*mail)
strbuf_addstr(, "\n\n");
if (strbuf_read_file(, am_path(state, "msg"), 0) < 0)
die_errno(_("could not read '%s'"), am_path(state, "msg"));
-   stripspace(, 0);
+   strbuf_stripspace(, 0);
 
if (state->signoff)
am_signoff();
diff --git a/builtin/branch.c b/builtin/branch.c
index 3ba4d1b..3f48746 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -794,7 +794,7 @@ static int edit_branch_description(const char *branch_name)
strbuf_release();
return -1;
}
-   stripspace(, 1);
+   strbuf_stripspace(, 1);
 
strbuf_addf(, "branch.%s.description", branch_name);
status = git_config_set(name.buf, buf.len ? buf.buf : NULL);
diff --git a/builtin/commit.c b/builtin/commit.c
index 63772d0..dca09e2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -775,7 +775,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
s->hints = 0;
 
if (clean_message_contents)
-   stripspace(, 0);
+   strbuf_stripspace(, 0);
 
if (signoff)
append_signoff(, ignore_non_trailer(), 0);
@@ -1014,7 +1014,7 @@ static int template_untouched(struct strbuf *sb)
if (!template_file || strbuf_read_file(, template_file, 0) <= 0)
return 0;
 
-   stripspace(, cleanup_mode == CLEANUP_ALL);
+   strbuf_stripspace(, cleanup_mode == CLEANUP_ALL);
if (!skip_prefix(sb->buf, tmpl.buf, ))
start = sb->buf;
strbuf_release();
@@ -1726,7 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
wt_status_truncate_message_at_cut_line();
 
if (cleanup_mode != CLEANUP_NONE)
-   stripspace(, cleanup_mode == CLEANUP_ALL);
+   strbuf_stripspace(, cleanup_mode == CLEANUP_ALL);
if (template_untouched() && !allow_empty_message) {
rollback_index_files();
fprintf(stderr, _("Aborting commit; you did not edit the 
message.\n"));
diff --git a/builtin/merge.c b/builtin/merge.c
index a0edaca..e6741f3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -806,7 +806,7 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
abort_commit(remoteheads, NULL);
}
read_merge_msg();
-   stripspace(, 0 < option_edit);
+   strbuf_stripspace(, 0 < option_edit);
if (!msg.len)
abort_commit(remoteheads, _("Empty commit message."));
strbuf_release(_msg);
diff --git a/builtin/notes.c b/builtin/notes.c
index 3608c64..bb23d55 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -192,7 +192,7 @@ static void prepare_note_data(const unsigned char *object, 
struct note_data *d,
if (launch_editor(d->edit_path, >buf, NULL)) {
die(_("Please supply the note contents using either -m 
or -F option"));
}
-   stripspace(>buf, 1);
+   strbuf_stripspace(>buf, 1);
}
 }
 
@@ -215,7 +215,7 @@ static int parse_msg_arg(const struct option *opt, const 
char *arg, int unset)
if (d->buf.len)
strbuf_addch(>buf, '\n');
strbuf_addstr(>buf, arg);
-   stripspace(>buf, 0);
+   strbuf_stripspace(>buf, 0);
 
d->given = 1;
return 0;
@@ -232,7 +232,7 @@ static int parse_file_arg(const struct option *opt, const 
char *arg, int unset)
die_errno(_("cannot read '%s'"), arg);
} else if (strbuf_read_file(>buf, arg, 1024) < 0)
die_errno(_("could not open or read '%s'"), arg);
-   stripspace(>buf, 0);
+   strbuf_stripspace(>buf, 0);
 
d->

[PATCH 2/3] stripspace: Implement --count-lines option

2015-10-15 Thread Tobias Klauser 
As suggested in the small project ideas [1], implement a --count-lines
options for git stripspace to be able to omit calling

  git stripspace --strip-comments < infile | wc -l

e.g. in git-rebase--interactive.sh. The above command can now be
replaced by:

  git stripspace --strip-comments --count-lines < infile

This will also make it easier to port git-rebase--interactive.sh to C
later on.

Furthermore, add the corresponding documentation and tests.

[1] 
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#implement_.27--count-lines.27_in_.27git_stripspace.27

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 Documentation/git-stripspace.txt | 13 -
 builtin/stripspace.c | 57 ++--
 strbuf.c | 12 ++---
 strbuf.h |  5 ++--
 t/t0030-stripspace.sh| 36 +
 5 files changed, 92 insertions(+), 31 deletions(-)

diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt
index 60328d5..411e17c 100644
--- a/Documentation/git-stripspace.txt
+++ b/Documentation/git-stripspace.txt
@@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace
 SYNOPSIS
 
 [verse]
-'git stripspace' [-s | --strip-comments] < input
+'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input
 'git stripspace' [-c | --comment-lines] < input
 
 DESCRIPTION
@@ -44,6 +44,11 @@ OPTIONS
be terminated with a newline. On empty lines, only the comment character
will be prepended.
 
+-C::
+--count-lines::
+   Output the number of resulting lines after stripping. This is equivalent
+   to calling 'git stripspace | wc -l'.
+
 EXAMPLES
 
 
@@ -88,6 +93,12 @@ Use 'git stripspace --strip-comments' to obtain:
 |The end.$
 -
 
+Use 'git stripspace --count-lines' to obtain:
+
+-
+|5$
+-
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index f677093..7882edd 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "strbuf.h"
 
 static void comment_lines(struct strbuf *buf)
@@ -12,45 +13,51 @@ static void comment_lines(struct strbuf *buf)
free(msg);
 }
 
-static const char *usage_msg = "\n"
-"  git stripspace [-s | --strip-comments] < input\n"
-"  git stripspace [-c | --comment-lines] < input";
+static const char * const usage_msg[] = {
+   N_("git stripspace [-s | --strip-comments] [-C | --count-lines] < 
input"),
+   N_("git stripspace [-c | --comment-lines] < input"),
+   NULL
+};
 
 int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
struct strbuf buf = STRBUF_INIT;
-   int strip_comments = 0;
-   enum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = 
STRIP_SPACE;
-
-   if (argc == 2) {
-   if (!strcmp(argv[1], "-s") ||
-   !strcmp(argv[1], "--strip-comments")) {
-   strip_comments = 1;
-   } else if (!strcmp(argv[1], "-c") ||
-  !strcmp(argv[1], "--comment-lines")) {
-   mode = COMMENT_LINES;
-   } else {
-   mode = INVAL;
-   }
-   } else if (argc > 1) {
-   mode = INVAL;
-   }
+   int comment_mode = 0, count_lines = 0, strip_comments = 0;
+   size_t lines = 0;
+
+   const struct option options[] = {
+   OPT_BOOL('s', "strip-comments", _comments,
+N_("skip and remove all lines starting with comment 
character")),
+   OPT_BOOL('c', "comment-lines", _mode,
+N_("prepend comment character and blank to each 
line")),
+   OPT_BOOL('C', "count-lines", _lines, N_("print line 
count")),
+   OPT_END()
+   };
 
-   if (mode == INVAL)
-   usage(usage_msg);
+   argc = parse_options(argc, argv, prefix, options, usage_msg,
+PARSE_OPT_KEEP_DASHDASH);
 
-   if (strip_comments || mode == COMMENT_LINES)
+   if (comment_mode && (count_lines || strip_comments))
+   usage_with_options(usage_msg, options);
+
+   if (strip_comments || comment_mode)
git_config(git_default_config, NULL);
 
if (strbuf_read(, 0, 1024) < 0)
die_errno("could not read the input");
 
-   if (mode == STRIP_SPACE)
-   strbuf_stripspace(, strip_comments);
+   if (!comment_mode)
+   lines = strbuf_stripspace(, strip_comments);
else

[PATCH 3/3] git rebase -i: Use newly added --count-lines option for stripspace

2015-10-15 Thread Tobias Klauser 
Use the newly added --count-lines option for 'git stripspace' to count
lines instead of piping the entire output to 'wc -l'.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 git-rebase--interactive.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f01637b..c40ca90 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -120,9 +120,9 @@ mark_action_done () {
sed -e 1q < "$todo" >> "$done"
sed -e 1d < "$todo" >> "$todo".new
mv -f "$todo".new "$todo"
-   new_count=$(git stripspace --strip-comments <"$done" | wc -l)
+   new_count=$(git stripspace --strip-comments --count-lines <"$done")
echo $new_count >"$msgnum"
-   total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc 
-l)))
+   total=$(($new_count + $(git stripspace --strip-comments --count-lines 
<"$todo")))
echo $total >"$end"
if test "$last_count" != "$new_count"
then
@@ -1247,7 +1247,7 @@ test -s "$todo" || echo noop >> "$todo"
 test -n "$autosquash" && rearrange_squash "$todo"
 test -n "$cmd" && add_exec_commands "$todo"
 
-todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
+todocount=$(git stripspace --strip-comments --count-lines <"$todo")
 todocount=${todocount##* }
 
 cat >>"$todo" <http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pretend_sha1_file(): Change return type from int to void

2015-10-07 Thread Tobias Klauser 
Hi Johannes

On 2015-10-06 at 16:30:36 +0200, Johannes Schindelin 
<johannes.schinde...@gmx.de> wrote:
> On 2015-10-06 15:51, Tobias Klauser wrote:
> 
> > On 2015-10-06 at 15:16:12 +0200, Johannes Schindelin
> > <johannes.schinde...@gmx.de> wrote:
> >>
> >> On 2015-10-06 14:15, Tobias Klauser wrote:
> >> > prented_sha1_file() always returns 0 and its only callsite in
> >> > builtin/blame.c doesn't use the return value, so change the return type
> >> > to void.
> >>
> >> While this commit message is technically correct, it would appear that 
> >> there are some things left unsaid.
> >>
> >> Is there a problem with the current code that is solved by not returning 
> >> 0? If so, could you add it to the commit message? And in particular, 
> >> change the oneline appropriately?
> > 
> > There's no problem with the current code other than that the return
> > value is unused and thus unnecessary for correct funcionality. So it's
> > certainly not a functional problem but rather a cosmetic change.
> 
> Okay.
> 
> > Does such a change even make sense (it's one of my first patch to git,
> > so I'm not really sure what your criteria in this respect are)?
> 
> Welcome!
> 
> As to the patch, I cannot speak for Junio, of course, but my preference would 
> be to keep the return type. Traditionally, functions that can fail either 
> die() or return an int; non-zero indicates an error. In this case, it seems 
> that we do not have any condition (yet...) under which an error could occur. 
> It does not seem very unlikely that we may eventually have such conditions, 
> though, hence my preference.

Ok, I see. Thank you for your explanation. I'll wait for Junio's decision
then :)

Cheers
Tobias
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pretend_sha1_file(): Change return type from int to void

2015-10-08 Thread Tobias Klauser 
On 2015-10-07 at 23:22:59 +0200, Junio C Hamano  wrote:
> Johannes Schindelin  writes:
> 
> > As to the patch, I cannot speak for Junio, of course, but my
> > preference would be to keep the return type. Traditionally, functions
> > that can fail either die() or return an int; non-zero indicates an
> > error. In this case, it seems that we do not have any condition
> > (yet...) under which an error could occur. It does not seem very
> > unlikely that we may eventually have such conditions, though, hence my
> > preference.
> 
> Perhaps the attached is a better approach.
> 
> Even though the current implementation of "pretend" implementation
> does not, future generations are allowed to make pretend_sha1_file()
> return failure when appropriate.

For my original patch I didn't consider that pretend_sha1_file() might
return failure in the future. I was just confused by the fact that the
return value was seemingly useless (but now I realize that unused !=
useless ;-), sorry for the noise.

Please disregard my patch and apply yours instead, if you see fit.

> 
>  builtin/blame.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 203a981..fa24f8f 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2362,7 +2362,8 @@ static struct commit *fake_working_tree_commit(struct 
> diff_options *opt,
>   convert_to_git(path, buf.buf, buf.len, , 0);
>   origin->file.ptr = buf.buf;
>   origin->file.size = buf.len;
> - pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);
> + if (pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1))
> + die("failed to create a fake commit for the working tree 
> version.");
>  
>   /*
>* Read the current index, replace the path entry with
> 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] stripspace: Implement --count-lines option

2015-10-16 Thread Tobias Klauser 
On 2015-10-15 at 18:52:54 +0200, Matthieu Moy <matthieu@grenoble-inp.fr> 
wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> > --- a/Documentation/git-stripspace.txt
> > +++ b/Documentation/git-stripspace.txt
> > @@ -9,7 +9,7 @@ git-stripspace - Remove unnecessary whitespace
> >  SYNOPSIS
> >  
> >  [verse]
> > -'git stripspace' [-s | --strip-comments] < input
> > +'git stripspace' [-s | --strip-comments] [-C | --count-lines] < input
> 
> I'm not sure it's a good idea to introduce a one-letter shortcut (-C).
> In scripts, --count-lines is self-explanatory hence more readable than
> -C (which is even more confusing since "git -C foo stripspace" and "git
> stripspace -C" have totally different meanings. Outside scripts, I'm not
> sure the command would be useful. I'd rather avoid polluting the
> one-letter-option namespace.

Ok, I'll drop the -C. Didn't consider the `git -C stripspace' case, so
that's definitely unwanted.

> > +Use 'git stripspace --count-lines' to obtain:
> > +
> > +-
> > +|5$
> > +-
> 
> In the examples above, I read the | as part of the input (unlike $ which
> is used only to show the end of line). So the | should not be here. I
> don't think you need the $ either, the --count-lines option is no longer
> about trailing whitespaces.

Will drop both | and $. Seems I didn't check the output careful enough,
both don't make sense for this option.

> > +static const char * const usage_msg[] = {
> 
> Stick the * to usage_msg please.

Will change in v2.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf

2015-10-16 Thread Tobias Klauser 
Thanks for the review.

On 2015-10-15 at 19:36:17 +0200, Junio C Hamano <gits...@pobox.com> wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> > Rename stripspace() to strbuf_stripspace() and move it to the strbuf
> > module as suggested in [1].
> >
> > Also switch all current users of stripspace() to the new function name
> > and  keep a temporary wrapper inline function for topic branches still
> > using stripspace().
> >
> > [1] 
> > https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf
> 
> I think Matthieu already mentioned this, but please explain why it
> is a good idea to do this in your own words here, without forcing
> readers to go to other places to find out.  Giving credit to an
> external page for giving you an inspiration to do something is good,
> but the proposed log message needs to justify itself.  In other
> words, when you are challenged to defend this change, you are not
> allowed to say "SmallProjectIdeas page said it is a good thing to
> do.  I just did it without questioning it." ;-)  Instead you are
> expected to justify it yourself.

Yes, makes sense. I'll adjust the commit message for v2 to justify the
change for itself and move the link below --- as Matthieu suggested.

> > Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
> > ---
> 
> A good rule of thumb to see if it is a good time to do this kind of
> change is to do this:
> 
>   $ git log -p maint..pu | grep stripspace
> 
> which shows only one mention in a context, so this change may cause
> textual conflict with something else somewhere nearby but won't hurt
> other topics in flight.

Ok, thanks for the hint. I'll check again before resubmitting v2.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] strbuf: make stripspace() part of strbuf

2015-10-16 Thread Tobias Klauser 
On 2015-10-15 at 18:42:23 +0200, Matthieu Moy <matthieu@grenoble-inp.fr> 
wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> > Also switch all current users of stripspace() to the new function name
> > and  keep a temporary wrapper inline function for topic branches still
> > using stripspace().
> 
> Since you have this temporary wrapper, it would have made sense to split
> the patch into 1) move and rename the function, and 2) change the
> callsites to strbuf_stripspace. This way 2) would be absolutely trivial
> to review.
> 
> OTOH, this patch is already easy to review, so you may consider it's OK
> like this.

Ok, in this case will keep the patch as is for v2, but with the adjusted
commit message as indicated in your and Junio's review.

> Reviewed-by: Matthieu Moy <matthieu@imag.fr>

Will add it to v2, thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] stripspace: Implement --count-lines option

2015-10-16 Thread Tobias Klauser 
On 2015-10-15 at 18:52:54 +0200, Matthieu Moy <matthieu@grenoble-inp.fr> 
wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> > +static const char * const usage_msg[] = {
> 
> Stick the * to usage_msg please.

Just noticed while looking at how other sub-commands define this, the vast
majority use "const char * const" and not "const char const *".

Also it would change the meaning. The following wouldn't produce a
compiler warning:

static const char const *usage_msg[] = { ... };
...
usage_msg[0] = "Foobar"

while

static const char * const usage_msg[] = { ... };
...
usage_msg[0] = "Foobar"

would produce one:

builtin/stripspace.c:36:2: error: assignment of read-only location 
‘usage_msg[0]’

Even though I don't expect anyone to modify usage_msg at runtime I think
it'd be better to stick with the current version.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/4] stripspace: Implement and use --count-lines option

2015-10-16 Thread Tobias Klauser 
(1) Move the stripspace() function to the strbuf module adding a prefix
and changing all users accordingly. Also introduce a wrapper in case
any topic branches still depend on the old name.

(2) Switch git stripspace to use parse-options in order to simplify
introducing new command line options (as in the following patch). In
v1 this was folded into patch (3) and is now split out for v2.

(3) Introduce option --count-lines to git stripspace and add the
corresponding documentation and tests.

(4) Change git-rebase--interactive.sh to replace commands like:

git stripspace ... | wc -l

with:

git stripspace --count-lines ...

This patch set implements some of the project ideas around git stripspace
suggested on https://git.wiki.kernel.org/index.php/SmallProjectsIdeas

v1 -> v2:

  - Thanks to Junio and Matthieu for the review.
  - Split patch 2/3 into two patches: patch 2/4 switches git stripspace
to use parse-options and patch 3/4 introduces the new option.
  - Implement line counting in cmd_stripbuf() instead of (ab-)using
strbuf_stripspace() for it.
  - Drop -C short option
  - Correct example command output in documentation.
  - Adjust commit messages to not include links to the wiki, fully
describe the motivation in the commit message instead.

Tobias Klauser (4):
  strbuf: make stripspace() part of strbuf
  stripspace: Use parse-options for command-line parsing
  stripspace: Implement --count-lines option
  git rebase -i: Use newly added --count-lines option for stripspace

 Documentation/git-stripspace.txt |  14 +++-
 builtin/am.c |   2 +-
 builtin/branch.c |   2 +-
 builtin/commit.c |   6 +-
 builtin/merge.c  |   2 +-
 builtin/notes.c  |   6 +-
 builtin/stripspace.c | 137 +--
 builtin/tag.c|   2 +-
 git-rebase--interactive.sh   |   6 +-
 strbuf.c |  66 +++
 strbuf.h |  11 +++-
 t/t0030-stripspace.sh|  36 ++
 12 files changed, 181 insertions(+), 109 deletions(-)

-- 
2.6.1.148.g7927db1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/4] stripspace: Implement --count-lines option

2015-10-16 Thread Tobias Klauser 
Implement the --count-lines options for git stripspace to be able to
omit calling:

  git stripspace --strip-comments < infile | wc -l

e.g. in git-rebase--interactive.sh. The above command can now be
replaced by:

  git stripspace --strip-comments --count-lines < infile

This will make it easier to port git-rebase--interactive.sh to C later
on.

Furthermore, add the corresponding documentation and tests.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---

Implements the small project idea from
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#implement_.27--count-lines.27_in_.27git_stripspace.27

 Documentation/git-stripspace.txt | 14 --
 builtin/stripspace.c | 18 +++---
 t/t0030-stripspace.sh| 36 
 3 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-stripspace.txt b/Documentation/git-stripspace.txt
index 60328d5..79900b8 100644
--- a/Documentation/git-stripspace.txt
+++ b/Documentation/git-stripspace.txt
@@ -9,8 +9,8 @@ git-stripspace - Remove unnecessary whitespace
 SYNOPSIS
 
 [verse]
-'git stripspace' [-s | --strip-comments] < input
-'git stripspace' [-c | --comment-lines] < input
+'git stripspace' [-s | --strip-comments] [--count-lines] < input
+'git stripspace' [-c | --comment-lines] [--count-lines] < input
 
 DESCRIPTION
 ---
@@ -44,6 +44,10 @@ OPTIONS
be terminated with a newline. On empty lines, only the comment character
will be prepended.
 
+--count-lines::
+   Output the number of resulting lines after stripping. This is equivalent
+   to calling 'git stripspace | wc -l'.
+
 EXAMPLES
 
 
@@ -88,6 +92,12 @@ Use 'git stripspace --strip-comments' to obtain:
 |The end.$
 -
 
+Use 'git stripspace --count-lines' to obtain:
+
+-
+5
+-
+
 GIT
 ---
 Part of the linkgit:git[1] suite
diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index ac1ab3d..487523f 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -14,8 +14,8 @@ static void comment_lines(struct strbuf *buf)
 }
 
 static const char * const stripspace_usage[] = {
-   N_("git stripspace [-s | --strip-comments] < input"),
-   N_("git stripspace [-c | --comment-lines] < input"),
+   N_("git stripspace [-s | --strip-comments] [--count-lines] < input"),
+   N_("git stripspace [-c | --comment-lines] [--count-lines] < input"),
NULL
 };
 
@@ -29,6 +29,7 @@ int cmd_stripspace(int argc, const char **argv, const char 
*prefix)
 {
struct strbuf buf = STRBUF_INIT;
enum stripspace_mode mode = STRIP_DEFAULT;
+   int count_lines = 0;
 
const struct option options[] = {
OPT_CMDMODE('s', "strip-comments", ,
@@ -37,6 +38,7 @@ int cmd_stripspace(int argc, const char **argv, const char 
*prefix)
OPT_CMDMODE('c', "comment-lines", ,
N_("prepend comment character and blank to each 
line"),
COMMENT_LINES),
+   OPT_BOOL(0, "count-lines", _lines, N_("print line 
count")),
OPT_END()
};
 
@@ -54,7 +56,17 @@ int cmd_stripspace(int argc, const char **argv, const char 
*prefix)
else
comment_lines();
 
-   write_or_die(1, buf.buf, buf.len);
+   if (!count_lines)
+   write_or_die(1, buf.buf, buf.len);
+   else {
+   size_t i, lines;
+
+   for (i = lines = 0; i < buf.len; i++) {
+   if (buf.buf[i] == '\n')
+   lines++;
+   }
+   printf("%zu\n", lines);
+   }
strbuf_release();
return 0;
 }
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 29e91d8..9c00cb9 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -438,4 +438,40 @@ test_expect_success 'avoid SP-HT sequence in commented 
line' '
test_cmp expect actual
 '
 
+test_expect_success '--count-lines with newline only' '
+   printf "0\n" >expect &&
+   printf "\n" | git stripspace --count-lines >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success '--count-lines with single line' '
+   printf "1\n" >expect &&
+   printf "foo\n" | git stripspace --count-lines >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success '--count-lines with single line preceeded by empty line' '
+   printf "1\n" >expect &&
+   printf "\nfoo" | git stripspace --count-lines >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success '--count-lines with single line followed by empty line' '
+   printf "1\n" >expect &&
+   prin

[PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

2015-10-16 Thread Tobias Klauser 
Use parse-options to parse command-line options instead of a
hand-crafted implementation.

This is a preparatory patch to simplify the introduction of the
--count-lines option in a follow-up patch.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 builtin/stripspace.c | 56 
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/builtin/stripspace.c b/builtin/stripspace.c
index f677093..ac1ab3d 100644
--- a/builtin/stripspace.c
+++ b/builtin/stripspace.c
@@ -1,5 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "strbuf.h"
 
 static void comment_lines(struct strbuf *buf)
@@ -12,41 +13,44 @@ static void comment_lines(struct strbuf *buf)
free(msg);
 }
 
-static const char *usage_msg = "\n"
-"  git stripspace [-s | --strip-comments] < input\n"
-"  git stripspace [-c | --comment-lines] < input";
+static const char * const stripspace_usage[] = {
+   N_("git stripspace [-s | --strip-comments] < input"),
+   N_("git stripspace [-c | --comment-lines] < input"),
+   NULL
+};
+
+enum stripspace_mode {
+   STRIP_DEFAULT = 0,
+   STRIP_COMMENTS,
+   COMMENT_LINES
+};
 
 int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
struct strbuf buf = STRBUF_INIT;
-   int strip_comments = 0;
-   enum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = 
STRIP_SPACE;
-
-   if (argc == 2) {
-   if (!strcmp(argv[1], "-s") ||
-   !strcmp(argv[1], "--strip-comments")) {
-   strip_comments = 1;
-   } else if (!strcmp(argv[1], "-c") ||
-  !strcmp(argv[1], "--comment-lines")) {
-   mode = COMMENT_LINES;
-   } else {
-   mode = INVAL;
-   }
-   } else if (argc > 1) {
-   mode = INVAL;
-   }
-
-   if (mode == INVAL)
-   usage(usage_msg);
-
-   if (strip_comments || mode == COMMENT_LINES)
+   enum stripspace_mode mode = STRIP_DEFAULT;
+
+   const struct option options[] = {
+   OPT_CMDMODE('s', "strip-comments", ,
+   N_("skip and remove all lines starting with comment 
character"),
+   STRIP_COMMENTS),
+   OPT_CMDMODE('c', "comment-lines", ,
+   N_("prepend comment character and blank to each 
line"),
+   COMMENT_LINES),
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, prefix, options, stripspace_usage,
+PARSE_OPT_KEEP_DASHDASH);
+
+   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
git_config(git_default_config, NULL);
 
if (strbuf_read(, 0, 1024) < 0)
die_errno("could not read the input");
 
-   if (mode == STRIP_SPACE)
-   strbuf_stripspace(, strip_comments);
+   if (mode == STRIP_DEFAULT || mode == STRIP_COMMENTS)
+   strbuf_stripspace(, mode == STRIP_COMMENTS);
else
comment_lines();
 
-- 
2.6.1.148.g7927db1


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/4] strbuf: make stripspace() part of strbuf

2015-10-16 Thread Tobias Klauser 
Rename stripspace() to strbuf_stripspace() and move it to the strbuf
module. The function is also used in other builtins than stripspace, so
it makes sense to have it in a more generic place. Since it operates on
an strbuf and the function is declared in strbuf.h, move it to strbuf.c
and add the corresponding prefix to its name.

Also switch all current users of stripspace() to the new function name
and keep a temporary wrapper inline function for any topic branches
still using stripspace().

Reviewed-by: Matthieu Moy <matthieu@imag.fr>
Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---

Implements the small project idea from
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#make_.27stripspace.28.29.27_part_of_strbuf

 builtin/am.c |  2 +-
 builtin/branch.c |  2 +-
 builtin/commit.c |  6 ++---
 builtin/merge.c  |  2 +-
 builtin/notes.c  |  6 ++---
 builtin/stripspace.c | 69 ++--
 builtin/tag.c|  2 +-
 strbuf.c | 66 +
 strbuf.h | 11 -
 9 files changed, 88 insertions(+), 78 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 3bd4fd7..7b8e11e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1343,7 +1343,7 @@ static int parse_mail(struct am_state *state, const char 
*mail)
strbuf_addstr(, "\n\n");
if (strbuf_read_file(, am_path(state, "msg"), 0) < 0)
die_errno(_("could not read '%s'"), am_path(state, "msg"));
-   stripspace(, 0);
+   strbuf_stripspace(, 0);
 
if (state->signoff)
am_signoff();
diff --git a/builtin/branch.c b/builtin/branch.c
index 01f9530..b99a436 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -592,7 +592,7 @@ static int edit_branch_description(const char *branch_name)
strbuf_release();
return -1;
}
-   stripspace(, 1);
+   strbuf_stripspace(, 1);
 
strbuf_addf(, "branch.%s.description", branch_name);
status = git_config_set(name.buf, buf.len ? buf.buf : NULL);
diff --git a/builtin/commit.c b/builtin/commit.c
index 63772d0..dca09e2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -775,7 +775,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
s->hints = 0;
 
if (clean_message_contents)
-   stripspace(, 0);
+   strbuf_stripspace(, 0);
 
if (signoff)
append_signoff(, ignore_non_trailer(), 0);
@@ -1014,7 +1014,7 @@ static int template_untouched(struct strbuf *sb)
if (!template_file || strbuf_read_file(, template_file, 0) <= 0)
return 0;
 
-   stripspace(, cleanup_mode == CLEANUP_ALL);
+   strbuf_stripspace(, cleanup_mode == CLEANUP_ALL);
if (!skip_prefix(sb->buf, tmpl.buf, ))
start = sb->buf;
strbuf_release();
@@ -1726,7 +1726,7 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
wt_status_truncate_message_at_cut_line();
 
if (cleanup_mode != CLEANUP_NONE)
-   stripspace(, cleanup_mode == CLEANUP_ALL);
+   strbuf_stripspace(, cleanup_mode == CLEANUP_ALL);
if (template_untouched() && !allow_empty_message) {
rollback_index_files();
fprintf(stderr, _("Aborting commit; you did not edit the 
message.\n"));
diff --git a/builtin/merge.c b/builtin/merge.c
index a0edaca..e6741f3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -806,7 +806,7 @@ static void prepare_to_commit(struct commit_list 
*remoteheads)
abort_commit(remoteheads, NULL);
}
read_merge_msg();
-   stripspace(, 0 < option_edit);
+   strbuf_stripspace(, 0 < option_edit);
if (!msg.len)
abort_commit(remoteheads, _("Empty commit message."));
strbuf_release(_msg);
diff --git a/builtin/notes.c b/builtin/notes.c
index 3608c64..bb23d55 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -192,7 +192,7 @@ static void prepare_note_data(const unsigned char *object, 
struct note_data *d,
if (launch_editor(d->edit_path, >buf, NULL)) {
die(_("Please supply the note contents using either -m 
or -F option"));
}
-   stripspace(>buf, 1);
+   strbuf_stripspace(>buf, 1);
}
 }
 
@@ -215,7 +215,7 @@ static int parse_msg_arg(const struct option *opt, const 
char *arg, int unset)
if (d->buf.len)
strbuf_addch(>buf, '\n');
strbuf_addstr(>buf, arg);
-   stripspace(>buf, 0);
+   strbuf_stripspace(>buf, 0);
 
d->given = 1;
return 0;
@@ -232,7 +232,7 @@ static int parse_file_arg(const struct option

[PATCH v2 4/4] git rebase -i: Use newly added --count-lines option for stripspace

2015-10-16 Thread Tobias Klauser 
Use the newly added --count-lines option for 'git stripspace' to count
lines instead of piping the entire output to 'wc -l'.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---

Implements the small project idea from
https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#implement_.27--count-lines.27_in_.27git_stripspace.27

 git-rebase--interactive.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d65c06e..f80da30 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -120,9 +120,9 @@ mark_action_done () {
sed -e 1q < "$todo" >> "$done"
sed -e 1d < "$todo" >> "$todo".new
mv -f "$todo".new "$todo"
-   new_count=$(git stripspace --strip-comments <"$done" | wc -l)
+   new_count=$(git stripspace --strip-comments --count-lines <"$done")
echo $new_count >"$msgnum"
-   total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc 
-l)))
+   total=$(($new_count + $(git stripspace --strip-comments --count-lines 
<"$todo")))
echo $total >"$end"
if test "$last_count" != "$new_count"
then
@@ -1243,7 +1243,7 @@ test -s "$todo" || echo noop >> "$todo"
 test -n "$autosquash" && rearrange_squash "$todo"
 test -n "$cmd" && add_exec_commands "$todo"
 
-todocount=$(git stripspace --strip-comments <"$todo" | wc -l)
+todocount=$(git stripspace --strip-comments --count-lines <"$todo")
 todocount=${todocount##* }
 
 cat >>"$todo" <http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

2015-10-20 Thread Tobias Klauser 
On 2015-10-17 at 23:24:13 +0200, Junio C Hamano <gits...@pobox.com> wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> > On 2015-10-16 at 19:29:35 +0200, Junio C Hamano <gits...@pobox.com> wrote:
> >> Junio C Hamano <gits...@pobox.com> writes:
> >> 
> >> >> -   if (mode == INVAL)
> >> >> -   usage(usage_msg);
> >> >
> >> > When given "git stripspace -s blorg", we used to set mode to INVAL
> >> > and then showed the correct usage.  But we no longer have a check
> >> > that corresponds to the old INVAL thing, do we?  Perhaps check argc
> >> > to detect presence of an otherwise ignored non-option argument
> >> > immediately after parse_options() returns?
> >> 
> >> Perhaps like this.
> >
> > Thanks. I'll fold it into v3.
> 
> Before starting v3, please fetch from me and check what is queued on
> 'pu'.  It may turn out that the fix-ups I did while queuing this
> round is sufficient, in which case you can just say that instead ;-)

Now that patches 3 and 4 will be dropped and no changes being necessary
for patches 1 and 2 (except for your changes that I see are already
folded into 'pu'), do you want me to submit a v3 of the series? Or is it
enough if I ask you to drop patches 3 (stripspace: implement
--count-lines option) and 4 (rebase -i: use "stripspace --count-lines"
when counting todo items)?

Thanks
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

2015-10-17 Thread Tobias Klauser 
On 2015-10-16 at 19:07:34 +0200, Junio C Hamano <gits...@pobox.com> wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> > Use parse-options to parse command-line options instead of a
> > hand-crafted implementation.
> >
> > This is a preparatory patch to simplify the introduction of the
> > --count-lines option in a follow-up patch.
> 
> The second paragraph is probably of much lessor importance than one
> thing you forgot to mention: the users can now use a unique prefix
> of the option and say "stripspace --comment".

I didn't even know about that feature, but now that you've mentioned it
I will certainly make use of it more in the future :) And of course,
I'll adjust the commit message accordingly for v3.

> > +enum stripspace_mode {
> > +   STRIP_DEFAULT = 0,
> > +   STRIP_COMMENTS,
> > +   COMMENT_LINES
> > +};
> >  
> >  int cmd_stripspace(int argc, const char **argv, const char *prefix)
> >  {
> > struct strbuf buf = STRBUF_INIT;
> > -   int strip_comments = 0;
> > -   enum { INVAL = 0, STRIP_SPACE = 1, COMMENT_LINES = 2 } mode = 
> > STRIP_SPACE;
> > -
> > -   if (argc == 2) {
> > -   if (!strcmp(argv[1], "-s") ||
> > -   !strcmp(argv[1], "--strip-comments")) {
> > -   strip_comments = 1;
> > -   } else if (!strcmp(argv[1], "-c") ||
> > -  !strcmp(argv[1], "--comment-lines")) {
> > -   mode = COMMENT_LINES;
> > -   } else {
> > -   mode = INVAL;
> > -   }
> > -   } else if (argc > 1) {
> > -   mode = INVAL;
> > -   }
> > -
> > -   if (mode == INVAL)
> > -   usage(usage_msg);
> 
> When given "git stripspace -s blorg", we used to set mode to INVAL
> and then showed the correct usage.  But we no longer have a check
> that corresponds to the old INVAL thing, do we?  Perhaps check argc
> to detect presence of an otherwise ignored non-option argument
> immediately after parse_options() returns?

Agreed, we should check it. I'll go with the implementation you
suggested in the follow-up message.

> > -   if (strip_comments || mode == COMMENT_LINES)
> > +   enum stripspace_mode mode = STRIP_DEFAULT;
> > +
> > +   const struct option options[] = {
> > +   OPT_CMDMODE('s', "strip-comments", ,
> > +   N_("skip and remove all lines starting with comment 
> > character"),
> > +   STRIP_COMMENTS),
> > +   OPT_CMDMODE('c', "comment-lines", ,
> > +   N_("prepend comment character and blank to each 
> > line"),
> > +   COMMENT_LINES),
> > +   OPT_END()
> > +   };
> > +
> > +   argc = parse_options(argc, argv, prefix, options, stripspace_usage,
> > +PARSE_OPT_KEEP_DASHDASH);
> 
> What is the point of keep-dashdash here?

Likewise, it shouldn't be there as in your follow-up patch.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] stripspace: Use parse-options for command-line parsing

2015-10-17 Thread Tobias Klauser 
On 2015-10-16 at 19:29:35 +0200, Junio C Hamano  wrote:
> Junio C Hamano  writes:
> 
> >> -  if (mode == INVAL)
> >> -  usage(usage_msg);
> >
> > When given "git stripspace -s blorg", we used to set mode to INVAL
> > and then showed the correct usage.  But we no longer have a check
> > that corresponds to the old INVAL thing, do we?  Perhaps check argc
> > to detect presence of an otherwise ignored non-option argument
> > immediately after parse_options() returns?
> 
> Perhaps like this.

Thanks. I'll fold it into v3.

> diff --git a/builtin/stripspace.c b/builtin/stripspace.c
> index ac1ab3d..a8b7a93 100644
> --- a/builtin/stripspace.c
> +++ b/builtin/stripspace.c
> @@ -40,8 +40,9 @@ int cmd_stripspace(int argc, const char **argv, const char 
> *prefix)
>   OPT_END()
>   };
>  
> - argc = parse_options(argc, argv, prefix, options, stripspace_usage,
> -  PARSE_OPT_KEEP_DASHDASH);
> + argc = parse_options(argc, argv, prefix, options, stripspace_usage, 0);
> + if (argc)
> + usage_with_options(stripspace_usage, options);
>  
>   if (mode == STRIP_COMMENTS || mode == COMMENT_LINES)
>   git_config(git_default_config, NULL);
> 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/4] stripspace: Implement and use --count-lines option

2015-10-17 Thread Tobias Klauser 
On 2015-10-16 at 18:41:31 +0200, Junio C Hamano <gits...@pobox.com> wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> Be consistent with the subjects, please.
> 
> >   strbuf: make stripspace() part of strbuf
> 
> s/make/make/ ;-)
> 
> >   stripspace: Use parse-options for command-line parsing
> 
> s/Use/use/
> 
> >   stripspace: Implement --count-lines option
> 
> s/Implement/implement/
> 
> >   git rebase -i: Use newly added --count-lines option for stripspace
> 
> s/Use/use/

Will adjust all of them to lowercase for v3. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/4] stripspace: Implement and use --count-lines option

2015-10-17 Thread Tobias Klauser 
On 2015-10-16 at 18:54:45 +0200, Matthieu Moy <matthieu@grenoble-inp.fr> 
wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> >   - Split patch 2/3 into two patches: patch 2/4 switches git stripspace
> > to use parse-options and patch 3/4 introduces the new option.
> 
> Much better now.
> 
> >   - Implement line counting in cmd_stripbuf() instead of (ab-)using
> > strbuf_stripspace() for it.
> 
> Also short and sweet, I like it.
> 
> >   - Drop -C short option
> >   - Correct example command output in documentation.
> >   - Adjust commit messages to not include links to the wiki, fully
> > describe the motivation in the commit message instead.
> 
> Good.
> 
> I read the patches again, and the whole series is now
> 
> Reviewed-by: Matthieu Moy <matthieu@imag.fr>

Thank you for the review!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] stripspace: Implement --count-lines option

2015-10-17 Thread Tobias Klauser 
On 2015-10-16 at 19:35:49 +0200, Junio C Hamano <gits...@pobox.com> wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> >> So this is your output code, which gives only the number of lines
> >> without the cleaned up result.
> >
> > This should better be a simple printf("%zu\n", lines) I guess?
> 
> I think we actively avoid using %z conversion that is only C99.
> 
> If you really want to, you could count in size_t and use %lu with
> appropriate casting, which I think is what we do in the rest of the
> codebase.
> 
> For this one, I think it is sufficient to just count in int and
> print as int with %d, though.

Ok, will use an int to count and printf("%d\n").
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Documentation/interpret-trailers: Grammar fix

2015-10-07 Thread Tobias Klauser 
Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 Documentation/git-interpret-trailers.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index d6d9231..0ecd497 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -67,7 +67,7 @@ OPTIONS
 --trim-empty::
If the  part of any trailer contains only whitespace,
the whole trailer will be removed from the resulting message.
-   This apply to existing trailers as well as new trailers.
+   This applies to existing trailers as well as new trailers.
 
 --trailer [(=|:)]::
Specify a (, ) pair that should be applied as a
-- 
2.6.0


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] connect: Fix typo in result string of prot_name()

2015-09-24 Thread Tobias Klauser 
Replace 'unkown' with 'unknown'.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 connect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/connect.c b/connect.c
index c0144d8..777f31c 100644
--- a/connect.c
+++ b/connect.c
@@ -254,7 +254,7 @@ static const char *prot_name(enum protocol protocol)
case PROTO_GIT:
return "git";
default:
-   return "unkown protocol";
+   return "unknown protocol";
}
 }
 
-- 
2.5.3


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pretend_sha1_file(): Change return type from int to void

2015-10-06 Thread Tobias Klauser 
prented_sha1_file() always returns 0 and its only callsite in
builtin/blame.c doesn't use the return value, so change the return type
to void.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 cache.h | 2 +-
 sha1_file.c | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 752031e..445853b 100644
--- a/cache.h
+++ b/cache.h
@@ -970,7 +970,7 @@ extern int sha1_object_info(const unsigned char *, unsigned 
long *);
 extern int hash_sha1_file(const void *buf, unsigned long len, const char 
*type, unsigned char *sha1);
 extern int write_sha1_file(const void *buf, unsigned long len, const char 
*type, unsigned char *return_sha1);
 extern int hash_sha1_file_literally(const void *buf, unsigned long len, const 
char *type, unsigned char *sha1, unsigned flags);
-extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned 
char *);
+extern void pretend_sha1_file(void *, unsigned long, enum object_type, 
unsigned char *);
 extern int force_object_loose(const unsigned char *sha1, time_t mtime);
 extern int git_open_noatime(const char *name);
 extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
diff --git a/sha1_file.c b/sha1_file.c
index d295a32..d76b723 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2789,14 +2789,14 @@ static void *read_packed_sha1(const unsigned char *sha1,
return data;
 }
 
-int pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
+void pretend_sha1_file(void *buf, unsigned long len, enum object_type type,
  unsigned char *sha1)
 {
struct cached_object *co;
 
hash_sha1_file(buf, len, typename(type), sha1);
if (has_sha1_file(sha1) || find_cached_object(sha1))
-   return 0;
+   return;
ALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc);
co = _objects[cached_object_nr++];
co->size = len;
@@ -2804,7 +2804,6 @@ int pretend_sha1_file(void *buf, unsigned long len, enum 
object_type type,
co->buf = xmalloc(len);
memcpy(co->buf, buf, len);
hashcpy(co->sha1, sha1);
-   return 0;
 }
 
 static void *read_object(const unsigned char *sha1, enum object_type *type,
-- 
2.6.0


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pretend_sha1_file(): Change return type from int to void

2015-10-06 Thread Tobias Klauser 
Hi Johannes

Thanks for your feedback.

On 2015-10-06 at 15:16:12 +0200, Johannes Schindelin 
<johannes.schinde...@gmx.de> wrote:
> Hi Tobias,
> 
> On 2015-10-06 14:15, Tobias Klauser wrote:
> > prented_sha1_file() always returns 0 and its only callsite in
> > builtin/blame.c doesn't use the return value, so change the return type
> > to void.
> 
> While this commit message is technically correct, it would appear that there 
> are some things left unsaid.
> 
> Is there a problem with the current code that is solved by not returning 0? 
> If so, could you add it to the commit message? And in particular, change the 
> oneline appropriately?

There's no problem with the current code other than that the return
value is unused and thus unnecessary for correct funcionality. So it's
certainly not a functional problem but rather a cosmetic change.

Does such a change even make sense (it's one of my first patch to git,
so I'm not really sure what your criteria in this respect are)?

If yes, would something like the following bring across the intention
more clearly?

  pretend_sha1_file() always returns 0 and its only user in
  builtin/blame.c doesn't use the returned value. Thus, the return value
  is unnecessary and the return type of pretend_sha1_file() can be
  changed to void.

Cheers
Tobias
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] interpret-trailers: add option for in-place editing

2016-01-06 Thread Tobias Klauser 
From: Tobias Klauser <tklau...@distanz.ch>

Add a command line option --in-place to support in-place editing akin to
sed -i.  This allows to write commands like the following:

  git interpret-trailers --trailer "X: Y" a.txt > b.txt && mv b.txt a.txt

in a more concise way:

  git interpret-trailers --trailer "X: Y" --in-place a.txt

Also add the corresponding documentation and tests.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 Documentation/git-interpret-trailers.txt | 24 +++-
 builtin/interpret-trailers.c | 13 +++
 t/t7513-interpret-trailers.sh| 32 ++
 trailer.c| 39 
 trailer.h|  3 ++-
 5 files changed, 91 insertions(+), 20 deletions(-)

diff --git a/Documentation/git-interpret-trailers.txt 
b/Documentation/git-interpret-trailers.txt
index 0ecd497c4de7..a77b901f1d7b 100644
--- a/Documentation/git-interpret-trailers.txt
+++ b/Documentation/git-interpret-trailers.txt
@@ -8,7 +8,7 @@ git-interpret-trailers - help add structured information into 
commit messages
 SYNOPSIS
 
 [verse]
-'git interpret-trailers' [--trim-empty] [(--trailer [(=|:)])...] 
[...]
+'git interpret-trailers' [--in-place] [--trim-empty] [(--trailer 
[(=|:)])...] [...]
 
 DESCRIPTION
 ---
@@ -64,6 +64,9 @@ folding rules, the encoding rules and probably many other 
rules.
 
 OPTIONS
 ---
+--in-place::
+   Edit the files in place.
+
 --trim-empty::
If the  part of any trailer contains only whitespace,
the whole trailer will be removed from the resulting message.
@@ -216,6 +219,25 @@ Signed-off-by: Alice <al...@example.com>
 Signed-off-by: Bob <b...@example.com>
 
 
+* Use the '--in-place' option to edit a message file in place:
++
+
+$ cat msg.txt
+subject
+
+message
+
+Signed-off-by: Bob <b...@example.com>
+$ git interpret-trailers --trailer 'Acked-by: Alice <al...@example.com>' 
--in-place msg.txt
+$ cat msg.txt
+subject
+
+message
+
+Signed-off-by: Bob <b...@example.com>
+Acked-by: Alice <al...@example.com>
+
+
 * Extract the last commit as a patch, and add a 'Cc' and a
   'Reviewed-by' trailer to it:
 +
diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
index 46838d24a90a..b99ae4be8875 100644
--- a/builtin/interpret-trailers.c
+++ b/builtin/interpret-trailers.c
@@ -12,16 +12,18 @@
 #include "trailer.h"
 
 static const char * const git_interpret_trailers_usage[] = {
-   N_("git interpret-trailers [--trim-empty] [(--trailer 
[(=|:)])...] [...]"),
+   N_("git interpret-trailers [--in-place] [--trim-empty] [(--trailer 
[(=|:)])...] [...]"),
NULL
 };
 
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
 {
+   int in_place = 0;
int trim_empty = 0;
struct string_list trailers = STRING_LIST_INIT_DUP;
 
struct option options[] = {
+   OPT_BOOL(0, "in-place", _place, N_("edit files in place")),
OPT_BOOL(0, "trim-empty", _empty, N_("trim empty 
trailers")),
OPT_STRING_LIST(0, "trailer", , N_("trailer"),
N_("trailer(s) to add")),
@@ -34,9 +36,12 @@ int cmd_interpret_trailers(int argc, const char **argv, 
const char *prefix)
if (argc) {
int i;
for (i = 0; i < argc; i++)
-   process_trailers(argv[i], trim_empty, );
-   } else
-   process_trailers(NULL, trim_empty, );
+   process_trailers(argv[i], in_place, trim_empty, 
);
+   } else {
+   if (in_place)
+   die(_("no input file given for in-place editing"));
+   process_trailers(NULL, in_place, trim_empty, );
+   }
 
string_list_clear(, 0);
 
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 322c436a494c..1103a4838b5c 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -326,6 +326,38 @@ test_expect_success 'with complex patch, args and 
--trim-empty' '
test_cmp expected actual
 '
 
+test_expect_success 'in-place editing with basic patch' '
+   cat basic_message >message &&
+   cat basic_patch >>message &&
+   cat basic_message >expected &&
+   echo >>expected &&
+   cat basic_patch >>expected &&
+   git interpret-trailers --in-place message &&
+   test_cmp expected message
+'
+
+test_expect_success 'in-place editing with additional trailer' '
+   cat basic_message >message &&
+   cat basic_patch >>message &&
+   cat ba

Re: [PATCH] interpret-trailers: add option for in-place editing

2016-01-06 Thread Tobias Klauser 
Thanks for your feedback Matthieu!

On 2016-01-06 at 15:19:45 +0100, Matthieu Moy <matthieu@grenoble-inp.fr> 
wrote:
> Tobias Klauser <tobias.klau...@zhinst.com> writes:
> 
> > From: Tobias Klauser <tklau...@distanz.ch>
> >
> > Add a command line option --in-place to support in-place editing akin to
> > sed -i.  This allows to write commands like the following:
> 
> Since -i is a common shortcut for --in-place (perl -i, sed -i), it
> probably makes sense to have it here too. OTOH, this is meant for
> scripting and perhaps it's best to force script writters to be verbose.

Yes, I thought this would mainly be used in scripts and thus omitted the
short option.

> > Also add the corresponding documentation and tests.
> 
> This sentence does not harm, but I personnally don't think it's really
> helpfull, as it's already clear by the diffstat right below, and the
> patch itself.

Ok, I can omit it for v2.

> > -static void print_tok_val(const char *tok, const char *val)
> > +static void print_tok_val(FILE *fp, const char *tok, const char *val)
> >  {
> > char c = last_non_space_char(tok);
> > if (!c)
> > return;
> > if (strchr(separators, c))
> > -   printf("%s%s\n", tok, val);
> > +   fprintf(fp, "%s%s\n", tok, val);
> > else
> > -   printf("%s%c %s\n", tok, separators[0], val);
> > +   fprintf(fp, "%s%c %s\n", tok, separators[0], val);
> >  }
> 
> The patch would be even easier to read if split into a preparatory
> refactoring patch "turn printf into fprintf" and then the actual one.
> But it's already rather clear, so you can probably leave it as-is.

Ok. I have also no problem with splitting it. I'll wait for a feedback
from Junio on what he prefers.

> > -static void print_lines(struct strbuf **lines, int start, int end)
> > +static void print_lines(FILE *fp, struct strbuf **lines, int start, int 
> > end)
> 
> Here and below: it would probably be more readable with a more explicit
> name for fp, like "outfile". Especially here:
> 
> > -static int process_input_file(struct strbuf **lines,
> > +static int process_input_file(FILE *fp,
> > + struct strbuf **lines,
> 
> Where it's tempting to think that a parameter given to
> process_input_file is ... the input file!

Yes, makes sense. I'll change it to a more concise and readable name
I'd also take "outfile" as you suggest, unless anyone objects.

Thanks
Tobias
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] interpret-trailers: add option for in-place editing

2016-01-07 Thread Tobias Klauser 
On 2016-01-06 at 20:02:23 +0100, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Wed, Jan 6, 2016 at 8:34 AM, Tobias Klauser
> <tobias.klau...@zhinst.com> wrote:
> > Add a command line option --in-place to support in-place editing akin to
> > sed -i.  This allows to write commands like the following:
> >
> >   git interpret-trailers --trailer "X: Y" a.txt > b.txt && mv b.txt a.txt
> >
> > in a more concise way:
> >
> >   git interpret-trailers --trailer "X: Y" --in-place a.txt
> >
> > Also add the corresponding documentation and tests.
> 
> In addition to Matthieu's comments...
> 
> > Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
> > ---
> > diff --git a/trailer.c b/trailer.c
> > @@ -856,19 +858,28 @@ void process_trailers(const char *file, int 
> > trim_empty, struct string_list *trai
> >
> > lines = read_input_file(file);
> >
> > +   if (in_place) {
> > +   fp = fopen(file, "w");
> > +   if (!fp)
> > +   die_errno(_("could not open file '%s' for 
> > writing"), file);
> > +   }
> 
> The input file should be considered precious, but this implementation
> plays too loosely with it. If the user interrupts the program or a
> die() somewhere within the "trailers" code aborts the program before
> the output file is written, then the original file will be
> irrecoverably lost. Users won't be happy about that.

Indeed, I didn't consider this. Thanks a lot for pointing this out.

> Instead, this code should go through the standard dance of writing the
> output to a new file (with some unique temporary name) and then, only
> once the output has been successfully written in full, rename the new
> file atop the old.

Ok, will do this for v2. I guess with the help of the functions from
tempfile.h it should be fairly easy to implement...

Thanks for your review!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] diffcore-delta: remove unused parameter to diffcore_count_changes()

2016-11-14 Thread Tobias Klauser 
The delta_limit parameter to diffcore_count_changes() has been unused
since commit ba23bbc8e ("diffcore-delta: make change counter to byte
oriented again.", 2006-03-04).

Remove the parameter and adjust all callers.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
v2: In the commit message, reference the correct commit where the parameter
usage was removed. Spotted by Jeff King.

 diff.c| 2 +-
 diffcore-break.c  | 1 -
 diffcore-delta.c  | 1 -
 diffcore-rename.c | 4 
 diffcore.h| 1 -
 5 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 8981477c436d..ec8728362dae 100644
--- a/diff.c
+++ b/diff.c
@@ -2023,7 +2023,7 @@ static void show_dirstat(struct diff_options *options)
if (DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {
diff_populate_filespec(p->one, 0);
diff_populate_filespec(p->two, 0);
-   diffcore_count_changes(p->one, p->two, NULL, NULL, 0,
+   diffcore_count_changes(p->one, p->two, NULL, NULL,
   , );
diff_free_filespec_data(p->one);
diff_free_filespec_data(p->two);
diff --git a/diffcore-break.c b/diffcore-break.c
index 881a74f29e4f..c64359f489c8 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -73,7 +73,6 @@ static int should_break(struct diff_filespec *src,
 
if (diffcore_count_changes(src, dst,
   >cnt_data, >cnt_data,
-  0,
   _copied, _added))
return 0;
 
diff --git a/diffcore-delta.c b/diffcore-delta.c
index 2ebedb32d18a..ebe70fb06851 100644
--- a/diffcore-delta.c
+++ b/diffcore-delta.c
@@ -166,7 +166,6 @@ int diffcore_count_changes(struct diff_filespec *src,
   struct diff_filespec *dst,
   void **src_count_p,
   void **dst_count_p,
-  unsigned long delta_limit,
   unsigned long *src_copied,
   unsigned long *literal_added)
 {
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 54a2396653df..f7444c86bde3 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -145,7 +145,6 @@ static int estimate_similarity(struct diff_filespec *src,
 * call into this function in that case.
 */
unsigned long max_size, delta_size, base_size, src_copied, 
literal_added;
-   unsigned long delta_limit;
int score;
 
/* We deal only with regular files.  Symlink renames are handled
@@ -191,11 +190,8 @@ static int estimate_similarity(struct diff_filespec *src,
if (!dst->cnt_data && diff_populate_filespec(dst, 0))
return 0;
 
-   delta_limit = (unsigned long)
-   (base_size * (MAX_SCORE-minimum_score) / MAX_SCORE);
if (diffcore_count_changes(src, dst,
   >cnt_data, >cnt_data,
-  delta_limit,
   _copied, _added))
return 0;
 
diff --git a/diffcore.h b/diffcore.h
index c11b8465fc8e..623024135478 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -142,7 +142,6 @@ extern int diffcore_count_changes(struct diff_filespec *src,
  struct diff_filespec *dst,
  void **src_count_p,
  void **dst_count_p,
- unsigned long delta_limit,
  unsigned long *src_copied,
  unsigned long *literal_added);
 
-- 
2.9.0

[PATCH] diffcore-delta: remove unused parameter to diffcore_count_changes()

2016-11-04 Thread Tobias Klauser 
The delta_limit parameter to diffcore_count_changes() has been unused
since commit c06c79667c95 ("diffcore-rename: somewhat optimized.").
Remove the parameter and adjust all callers.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 diff.c| 2 +-
 diffcore-break.c  | 1 -
 diffcore-delta.c  | 1 -
 diffcore-rename.c | 4 
 diffcore.h| 1 -
 5 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 8981477c436d..ec8728362dae 100644
--- a/diff.c
+++ b/diff.c
@@ -2023,7 +2023,7 @@ static void show_dirstat(struct diff_options *options)
if (DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {
diff_populate_filespec(p->one, 0);
diff_populate_filespec(p->two, 0);
-   diffcore_count_changes(p->one, p->two, NULL, NULL, 0,
+   diffcore_count_changes(p->one, p->two, NULL, NULL,
   , );
diff_free_filespec_data(p->one);
diff_free_filespec_data(p->two);
diff --git a/diffcore-break.c b/diffcore-break.c
index 881a74f29e4f..c64359f489c8 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -73,7 +73,6 @@ static int should_break(struct diff_filespec *src,
 
if (diffcore_count_changes(src, dst,
   >cnt_data, >cnt_data,
-  0,
   _copied, _added))
return 0;
 
diff --git a/diffcore-delta.c b/diffcore-delta.c
index 2ebedb32d18a..ebe70fb06851 100644
--- a/diffcore-delta.c
+++ b/diffcore-delta.c
@@ -166,7 +166,6 @@ int diffcore_count_changes(struct diff_filespec *src,
   struct diff_filespec *dst,
   void **src_count_p,
   void **dst_count_p,
-  unsigned long delta_limit,
   unsigned long *src_copied,
   unsigned long *literal_added)
 {
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 54a2396653df..f7444c86bde3 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -145,7 +145,6 @@ static int estimate_similarity(struct diff_filespec *src,
 * call into this function in that case.
 */
unsigned long max_size, delta_size, base_size, src_copied, 
literal_added;
-   unsigned long delta_limit;
int score;
 
/* We deal only with regular files.  Symlink renames are handled
@@ -191,11 +190,8 @@ static int estimate_similarity(struct diff_filespec *src,
if (!dst->cnt_data && diff_populate_filespec(dst, 0))
return 0;
 
-   delta_limit = (unsigned long)
-   (base_size * (MAX_SCORE-minimum_score) / MAX_SCORE);
if (diffcore_count_changes(src, dst,
   >cnt_data, >cnt_data,
-  delta_limit,
   _copied, _added))
return 0;
 
diff --git a/diffcore.h b/diffcore.h
index c11b8465fc8e..623024135478 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -142,7 +142,6 @@ extern int diffcore_count_changes(struct diff_filespec *src,
  struct diff_filespec *dst,
  void **src_count_p,
  void **dst_count_p,
- unsigned long delta_limit,
  unsigned long *src_copied,
  unsigned long *literal_added);
 
-- 
2.9.0

[PATCH] branch: remove unused parameter to create_branch()

2016-11-04 Thread Tobias Klauser 
The name parameter to create_branch() has been unused since commit
55c4a673070f ("Prevent force-updating of the current branch"). Remove
the parameter and adjust the callers accordingly. Also remove the
parameter from the function's documentation comment.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 branch.c   |  3 +--
 branch.h   | 15 +++
 builtin/branch.c   |  4 ++--
 builtin/checkout.c |  2 +-
 4 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/branch.c b/branch.c
index a5a8dcbd0ed9..0d459b3cfe50 100644
--- a/branch.c
+++ b/branch.c
@@ -228,8 +228,7 @@ N_("\n"
 "will track its remote counterpart, you may want to use\n"
 "\"git push -u\" to set the upstream config as you push.");
 
-void create_branch(const char *head,
-  const char *name, const char *start_name,
+void create_branch(const char *name, const char *start_name,
   int force, int reflog, int clobber_head,
   int quiet, enum branch_track track)
 {
diff --git a/branch.h b/branch.h
index b2f964933270..8e63d1b6f964 100644
--- a/branch.h
+++ b/branch.h
@@ -4,15 +4,14 @@
 /* Functions for acting on the information about branches. */
 
 /*
- * Creates a new branch, where head is the branch currently checked
- * out, name is the new branch name, start_name is the name of the
- * existing branch that the new branch should start from, force
- * enables overwriting an existing (non-head) branch, reflog creates a
- * reflog for the branch, and track causes the new branch to be
- * configured to merge the remote branch that start_name is a tracking
- * branch for (if any).
+ * Creates a new branch, where name is the new branch name, start_name
+ * is the name of the existing branch that the new branch should start
+ * from, force enables overwriting an existing (non-head) branch, reflog
+ * creates a reflog for the branch, and track causes the new branch to
+ * be configured to merge the remote branch that start_name is a
+ * tracking branch for (if any).
  */
-void create_branch(const char *head, const char *name, const char *start_name,
+void create_branch(const char *name, const char *start_name,
   int force, int reflog,
   int clobber_head, int quiet, enum branch_track track);
 
diff --git a/builtin/branch.c b/builtin/branch.c
index d5d93a8c03fe..60cc5c8e8da0 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -807,7 +807,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
 * create_branch takes care of setting up the tracking
 * info and making sure new_upstream is correct
 */
-   create_branch(head, branch->name, new_upstream, 0, 0, 0, quiet, 
BRANCH_TRACK_OVERRIDE);
+   create_branch(branch->name, new_upstream, 0, 0, 0, quiet, 
BRANCH_TRACK_OVERRIDE);
} else if (unset_upstream) {
struct branch *branch = branch_get(argv[0]);
struct strbuf buf = STRBUF_INIT;
@@ -853,7 +853,7 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
strbuf_release();
 
branch_existed = ref_exists(branch->refname);
-   create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
+   create_branch(argv[0], (argc == 2) ? argv[1] : head,
  force, reflog, 0, quiet, track);
 
/*
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9b2a5b31d423..512492aad909 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -630,7 +630,7 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
}
}
else
-   create_branch(old->name, opts->new_branch, new->name,
+   create_branch(opts->new_branch, new->name,
  opts->new_branch_force ? 1 : 0,
  opts->new_branch_log,
  opts->new_branch_force ? 1 : 0,
-- 
2.9.0

Re: [PATCH] branch: remove unused parameter to create_branch()

2016-11-04 Thread Tobias Klauser 
On 2016-11-04 at 17:30:12 +0100, Jeff King <p...@peff.net> wrote:
> On Fri, Nov 04, 2016 at 04:19:49PM +0100, Tobias Klauser wrote:
> 
> > The name parameter to create_branch() has been unused since commit
> > 55c4a673070f ("Prevent force-updating of the current branch"). Remove
> > the parameter and adjust the callers accordingly. Also remove the
> > parameter from the function's documentation comment.
> 
> This seemed eerily familiar, and it turns out I wrote this as a
> preparatory step for a different topic a while back, but never finished
> it.
> 
> So clearly a good change, though we might want to explain a bit more why
> it's correct that the parameter is unused. Here's what I wrote:
> 
>   This function used to have the caller pass in the current value of
>   HEAD, in order to make sure we didn't clobber HEAD.  In 55c4a6730,
>   that logic moved to validate_new_branchname(), which just resolves
>   HEAD itself. The parameter to create_branch is now unused.

Ah, I didn't know about the history of this parameter. It clearly makes
sense to explain this in the patch description.

> I also ended up reformatting the documentation comment, but that's
> purely optional. My patch is below for reference. Feel free to grab any
> bits of it that you agree with.

I like your documentation comment much better as IMO it's easier to read
and to identify the individual parameters.

Given these facts, I guess it's better if my patch is dropped and yours
is applied instead :)

Thanks a lot!

Re: [PATCH] diffcore-delta: remove unused parameter to diffcore_count_changes()

2016-11-04 Thread Tobias Klauser 
On 2016-11-04 at 17:37:14 +0100, Jeff King <p...@peff.net> wrote:
> On Fri, Nov 04, 2016 at 11:24:36AM +0100, Tobias Klauser wrote:
> 
> > The delta_limit parameter to diffcore_count_changes() has been unused
> > since commit c06c79667c95 ("diffcore-rename: somewhat optimized.").
> > Remove the parameter and adjust all callers.
> 
> Sounds like a good idea to get rid of an unused parameter, but I think
> this went away in ba23bbc8e (diffcore-delta: make change counter to byte
> oriented again., 2006-03-04).

Ugh, I must have fat-fingered the commit id. Will update the description
accordingly for v2.

Thanks!
Tobias

[PATCH] RelNotes: typo fix in 2.11.0 notes

2016-11-29 Thread Tobias Klauser 
s/paht/path/ in the "Backwards compatibility notes" section of the
2.11.0 release notes.

Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
---
 Documentation/RelNotes/2.11.0.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/RelNotes/2.11.0.txt 
b/Documentation/RelNotes/2.11.0.txt
index b7b7dd361ef0..4c8a9be60f52 100644
--- a/Documentation/RelNotes/2.11.0.txt
+++ b/Documentation/RelNotes/2.11.0.txt
@@ -5,7 +5,7 @@ Backward compatibility notes.
 
  * An empty string used as a pathspec element has always meant
'everything matches', but it is too easy to write a script that
-   finds a path to remove in $path and run 'git rm "$paht"' by
+   finds a path to remove in $path and run 'git rm "$path"' by
mistake (when the user meant to give "$path"), which ends up
removing everything.  This release starts warning about the
use of an empty string that is used for 'everything matches' and
-- 
2.11.0.rc3.5.g7cdf2ab.dirty

Re: [PATCH] RelNotes: typo fix in 2.11.0 notes

2016-11-29 Thread Tobias Klauser 
On 2016-11-29 at 19:35:38 +0100, Junio C Hamano <gits...@pobox.com> wrote:
> Tobias Klauser <tklau...@distanz.ch> writes:
> 
> > s/paht/path/ in the "Backwards compatibility notes" section of the
> > 2.11.0 release notes.
> >
> > Signed-off-by: Tobias Klauser <tklau...@distanz.ch>
> > ---
> 
> This looks somewhat familiar.  Perhaps
> 
>   https://public-inbox.org/git/1477668782.1869.4.ca...@seestieto.com/

Oops, certainly didn't read carefiul enough. Sorry for the noise and
thanks for the reference.

> >  Documentation/RelNotes/2.11.0.txt | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/RelNotes/2.11.0.txt 
> > b/Documentation/RelNotes/2.11.0.txt
> > index b7b7dd361ef0..4c8a9be60f52 100644
> > --- a/Documentation/RelNotes/2.11.0.txt
> > +++ b/Documentation/RelNotes/2.11.0.txt
> > @@ -5,7 +5,7 @@ Backward compatibility notes.
> >  
> >   * An empty string used as a pathspec element has always meant
> > 'everything matches', but it is too easy to write a script that
> > -   finds a path to remove in $path and run 'git rm "$paht"' by
> > +   finds a path to remove in $path and run 'git rm "$path"' by
> > mistake (when the user meant to give "$path"), which ends up
> > removing everything.  This release starts warning about the
> > use of an empty string that is used for 'everything matches' and
>

Re: [PATCH] strbuf: Remove unused stripspace function alias

2017-11-29 Thread Tobias Klauser 
On 2017-11-29 at 02:45:59 +0100, Elijah Newren <new...@gmail.com> wrote:
> In commit 63af4a8446 ("strbuf: make stripspace() part of strbuf",
> 2015-10-16), stripspace() was moved to strbuf and renamed to
> strbuf_stripspace().  A "temporary" alias was added for the old name until
> all topic branches had time to switch over.  They have had time, so remove
> the old alias.
> 
> Signed-off-by: Elijah Newren <new...@gmail.com>

Reviewed-by: Tobias Klauser <tklau...@distanz.ch>

Thanks!

[PATCH] git-rebase--{interactive,preserve-merges}: fix formatting of todo help message

2018-07-04 Thread Tobias Klauser 
Part of the todo help message in git-rebase--interactive.sh and
git-rebase--preserve-merges.sh is unnecessarily indented, making the
message look weird:

  # These lines can be re-ordered; they are executed from top to bottom.
  #
  # If you remove a line here THAT COMMIT WILL BE LOST.
  #
  #   However, if you remove everything, the rebase will be aborted.
  #
  #
  # Note that empty commits are commented out

Remove the extra lines and trailing indent to make it look as follows:

  # These lines can be re-ordered; they are executed from top to bottom.
  #
  # If you remove a line here THAT COMMIT WILL BE LOST.
  #
  # However, if you remove everything, the rebase will be aborted.
  #
  # Note that empty commits are commented out

Signed-off-by: Tobias Klauser 
---
 git-rebase--interactive.sh | 4 ++--
 git-rebase--preserve-merges.sh | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 299ded21375e..a31af6d4c419 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -222,9 +222,9 @@ $comment_char $(eval_ngettext \
 EOF
append_todo_help
gettext "
-   However, if you remove everything, the rebase will be aborted.
+However, if you remove everything, the rebase will be aborted.
 
-   " | git stripspace --comment-lines >>"$todo"
+" | git stripspace --comment-lines >>"$todo"
 
if test -z "$keep_empty"
then
diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh
index c51c7828e7b3..c214c5e4d6ce 100644
--- a/git-rebase--preserve-merges.sh
+++ b/git-rebase--preserve-merges.sh
@@ -891,9 +891,9 @@ $comment_char $(eval_ngettext \
 EOF
append_todo_help
gettext "
-   However, if you remove everything, the rebase will be aborted.
+However, if you remove everything, the rebase will be aborted.
 
-   " | git stripspace --comment-lines >>"$todo"
+" | git stripspace --comment-lines >>"$todo"
 
if test -z "$keep_empty"
then
-- 
2.18.0.130.g98da2f6b3e4a

Re: [PATCH] git-rebase--{interactive,preserve-merges}: fix formatting of todo help message

2018-07-04 Thread Tobias Klauser 
Hi Johannes

Thanks for your detailed answer.

On 2018-07-04 at 15:09:34 +0200, Johannes Schindelin 
 wrote:
> On Wed, 4 Jul 2018, Tobias Klauser wrote:
> 
> > Part of the todo help message in git-rebase--interactive.sh and
> > git-rebase--preserve-merges.sh is unnecessarily indented, making the
> > message look weird:
> > 
> >   # These lines can be re-ordered; they are executed from top to bottom.
> >   #
> >   # If you remove a line here THAT COMMIT WILL BE LOST.
> >   #
> >   #   However, if you remove everything, the rebase will be aborted.
> >   #
> >   #
> >   # Note that empty commits are commented out
> > 
> > Remove the extra lines and trailing indent to make it look as follows:
> > 
> >   # These lines can be re-ordered; they are executed from top to bottom.
> >   #
> >   # If you remove a line here THAT COMMIT WILL BE LOST.
> >   #
> >   # However, if you remove everything, the rebase will be aborted.
> >   #
> >   # Note that empty commits are commented out
> > 
> > Signed-off-by: Tobias Klauser 
> 
> Thank you for this contribution!
> 
> Funnily enough, the same fix had been provided here:
> https://public-inbox.org/git/614f0c12-7173-48bd-9212-71ad6fbbd...@dana.is/
> although only for `git-rebase--interactive.sh`.
> 
> In that thread, we agreed that it overlaps too much with a GSoC project,
> and that that project already addresses the same problem by way of
> rewriting that part in C (and therefore we decided it would be better to
> go with those patches).

Unfortunately I don't follow git development that closely anymore, so I
wasn't aware of the previous patch and the discussion. Sorry about the
duplication/conflict.

And I'm glad to see that this part will be rewritten in C :-)

> However, your patch also covers this:
> 
> >  git-rebase--preserve-merges.sh | 4 ++--
> 
> I completely missed that in the earlier patch.
> 
> Junio, this gets an ACK from me, could you apply the
> `git-rebase--preserve-merges.sh` part selectively, please?

Let me know if I should provide an updated patch just for
git-rebase--preserve-merges.sh

Thanks
Tobias