What's cooking in git.git (Jun 2010, #01; Wed, 2)

Tay Ray Chuan· Jun 3, 2010, 03:13 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

Hi,

On Thu, Jun 3, 2010 at 7:36 AM, Junio C Hamano <gitster@pobox.com> wrote:

> [New Topics]
> [snip]
> * tc/commit-abbrev-fix (2010-05-27) 3 commits
>  - commit::print_summary(): set rev_info.always_show_header to 1
>  - t7502-commit: add summary output tests for empty and merge commits
>  - t7502-commit: add tests for summary output
>
> Will merge to 'next'.  I am not quite happy about the "impossible to
> trigger" die message, though.  It is a good defensive programming to catch
> breakages caused by future changes that may invalidate the assumption this
> patch makes, but then the message should be worded as such to state that
> assumption, I think.

> [Cooking]
> [snip]
> * tc/merge-m-log (2010-05-11) 8 commits
>  (merged to 'next' on 2010-05-21 at e889876)
>  + merge: --log appends shortlog to message if specified
>  + fmt-merge-msg: add function to append shortlog only
>  + fmt-merge-msg: refactor merge title formatting
>  + fmt-merge-msg: minor refactor of fmt_merge_msg()
>  + merge: rename variable
>  + merge: update comment
>  + t7604-merge-custom-message: show that --log doesn't append to -m
>  + t7604-merge-custom-message: shift expected output creation

-- 
Cheers,
Ray Chuan

Tay Ray Chuan· Jun 4, 2010, 08:21 UTC · re: Tay Ray Chuan · lore

[PATCH v3 3/3] commit::print_summary(): set rev_info.always_show_header to 1

Quoting log-tree.c:560:

	shown = log_tree_diff(opt, commit, &log);
	if (!shown && opt->loginfo && opt->always_show_header) {
		log.parent = NULL;
		show_log(opt);
		shown = 1;
	}

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changes from v2:
 - shift around 2nd and 3rd paras.
 - mention the die() and our assumption that it won't be triggered, as
   suggested by Junio.

 builtin/commit.c  |   13 ++++---------
 t/t7502-commit.sh |    4 ++--
 2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a4e4966..2884d0c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1148,7 +1148,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	rev.verbose_header = 1;
 	rev.show_root_diff = 1;
 	get_commit_format(format.buf, &rev);
-	rev.always_show_header = 0;
+	rev.always_show_header = 1;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
 	rev.diffopt.break_opt = 0;
@@ -1162,14 +1162,9 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 				head,
 		initial_commit ? " (root-commit)" : "");

-	if (!log_tree_commit(&rev, commit)) {
-		struct pretty_print_context ctx = {0};
-		struct strbuf buf = STRBUF_INIT;
-		ctx.date_mode = DATE_NORMAL;
-		format_commit_message(commit, format.buf + 7, &buf, &ctx);
-		printf("%s\n", buf.buf);
-		strbuf_release(&buf);
-	}
+	if (!log_tree_commit(&rev, commit))
+		die("unable to print summary");
+
 	strbuf_release(&format);
 }

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index b10541d..08c0247 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
 	check_summary_oneline "" "a change"
 '

-test_expect_failure 'output summary format for commit with an empty diff' '
+test_expect_success 'output summary format for commit with an empty diff' '

 	check_summary_oneline "" "empty" "--allow-empty"
 '

-test_expect_failure 'output summary format for merges' '
+test_expect_success 'output summary format for merges' '

 	git checkout -b recursive-base &&
 	test_commit base file1 &&
--
1.7.1.189.g07419

Tay Ray Chuan· Jun 4, 2010, 08:34 UTC · re: Tay Ray Chuan · lore

Re: [PATCH v3 3/3] commit::print_summary(): set rev_info.always_show_header to 1

Hi Will,

sorry, I forgot to Cc you for this new version.

-- 
Cheers,
Ray Chuan

On Fri, Jun 4, 2010 at 4:21 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> This attempts to fix a regression in git-commit, where non-abbreviated
> SHA-1s were printed in the summary.
>
> One possible fix would be to set ctx.abbrev to DEFAULT_ABBREV in the
> `if` block.
>
> However, we remove this codeblock altogether, and set
> rev.always_show_header. This way, we use back the same show_log()
> mechanism (instead of format_commit_message()).
>
> Quoting log-tree.c:560:
>
>        shown = log_tree_diff(opt, commit, &log);
>        if (!shown && opt->loginfo && opt->always_show_header) {
>                log.parent = NULL;
>                show_log(opt);
>                shown = 1;
>        }
>
> This is the only area that always_show_header is checked, so the
> setting of this flag should only affect this area.
>
> Also, we now die() if log_tree_commit() returns false. Based on the
> existing underlying codepaths (log_tree_commit(), log_tree_diff(),
> log_tree_diff_flush(), to name a few), this should not occur; changes to
> these codepaths may warrant a revision of our handling of this
> situation. Tests #2 and #3 in t7502 should aid in detecting such
> breakages.
>
> Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
> ---
>
> This is a reworked version of the third patch of the
> 'tc/commit-abbrev-fix' series; there are no changes to the first and
> second patches.
>
> Changes from v2:
>  - shift around 2nd and 3rd paras.
>  - mention the die() and our assumption that it won't be triggered, as
>   suggested by Junio.
>
>  builtin/commit.c  |   13 ++++---------
>  t/t7502-commit.sh |    4 ++--
>  2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index a4e4966..2884d0c 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1148,7 +1148,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>        rev.verbose_header = 1;
>        rev.show_root_diff = 1;
>        get_commit_format(format.buf, &rev);
> -       rev.always_show_header = 0;
> +       rev.always_show_header = 1;
>        rev.diffopt.detect_rename = 1;
>        rev.diffopt.rename_limit = 100;
>        rev.diffopt.break_opt = 0;
> @@ -1162,14 +1162,9 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
>                                head,
>                initial_commit ? " (root-commit)" : "");
>
> -       if (!log_tree_commit(&rev, commit)) {
> -               struct pretty_print_context ctx = {0};
> -               struct strbuf buf = STRBUF_INIT;
> -               ctx.date_mode = DATE_NORMAL;
> -               format_commit_message(commit, format.buf + 7, &buf, &ctx);
> -               printf("%s\n", buf.buf);
> -               strbuf_release(&buf);
> -       }
> +       if (!log_tree_commit(&rev, commit))
> +               die("unable to print summary");
> +
>        strbuf_release(&format);
>  }
>
> diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
> index b10541d..08c0247 100755
> --- a/t/t7502-commit.sh
> +++ b/t/t7502-commit.sh
> @@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
>        check_summary_oneline "" "a change"
>  '
>
> -test_expect_failure 'output summary format for commit with an empty diff' '
> +test_expect_success 'output summary format for commit with an empty diff' '
>
>        check_summary_oneline "" "empty" "--allow-empty"
>  '
>
> -test_expect_failure 'output summary format for merges' '
> +test_expect_success 'output summary format for merges' '
>
>        git checkout -b recursive-base &&
>        test_commit base file1 &&
> --
> 1.7.1.189.g07419
>
>

Junio C Hamano· Jun 5, 2010, 06:44 UTC · re: Tay Ray Chuan · lore

Re: [PATCH v3 3/3] commit::print_summary(): set rev_info.always_show_header to 1

Tay Ray Chuan <rctay89@gmail.com> writes:

>  - mention the die() and our assumption that it won't be triggered, as
>    suggested by Junio.

Tay Ray Chuan· Jun 7, 2010, 05:04 UTC · re: Tay Ray Chuan · lore

[PATCH v4] commit::print_summary(): set rev_info.always_show_header to 1

Quoting log-tree.c:560:

	shown = log_tree_diff(opt, commit, &log);
	if (!shown && opt->loginfo && opt->always_show_header) {
		log.parent = NULL;
		show_log(opt);
		shown = 1;
	}

Signed-off-by: Tay Ray Chuan <rctay89@gmail.com>
---

Changes from v3:
 - shift note on the die() and our assumption from the commit message into the
   code (as a comment), as suggested by Junio.

 builtin/commit.c  |   22 +++++++++++++---------
 t/t7502-commit.sh |    4 ++--
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index a4e4966..b0c266c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1148,7 +1148,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	rev.verbose_header = 1;
 	rev.show_root_diff = 1;
 	get_commit_format(format.buf, &rev);
-	rev.always_show_header = 0;
+	rev.always_show_header = 1;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
 	rev.diffopt.break_opt = 0;
@@ -1162,14 +1162,18 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 				head,
 		initial_commit ? " (root-commit)" : "");

-	if (!log_tree_commit(&rev, commit)) {
-		struct pretty_print_context ctx = {0};
-		struct strbuf buf = STRBUF_INIT;
-		ctx.date_mode = DATE_NORMAL;
-		format_commit_message(commit, format.buf + 7, &buf, &ctx);
-		printf("%s\n", buf.buf);
-		strbuf_release(&buf);
-	}
+	/**
+	 * Based on the existing underlying codepaths (log_tree_commit(),
+	 * log_tree_diff(), log_tree_diff_flush(), to name a few), the die()
+	 * should not occur; changes to these codepaths may warrant a revision
+	 * of our handling of this situation.
+	 *
+	 * Tests #1-#3 in t7502 for output formats should aid in detecting such
+	 * breakages.
+	 */
+	if (!log_tree_commit(&rev, commit))
+		die("unable to print summary");
+
 	strbuf_release(&format);
 }

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index b10541d..08c0247 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -36,12 +36,12 @@ test_expect_success 'output summary format' '
 	check_summary_oneline "" "a change"
 '

-test_expect_failure 'output summary format for commit with an empty diff' '
+test_expect_success 'output summary format for commit with an empty diff' '

 	check_summary_oneline "" "empty" "--allow-empty"
 '

-test_expect_failure 'output summary format for merges' '
+test_expect_success 'output summary format for merges' '

 	git checkout -b recursive-base &&
 	test_commit base file1 &&
--
1.7.1.189.g07419

Michael J Gruber· Jun 3, 2010, 08:40 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

Junio C Hamano venit, vidit, dixit 03.06.2010 01:36:

> Here are the topics that have been cooking.  Commits prefixed with '-' are
> only in 'pu' while commits prefixed with '+' are in 'next'.  The ones
> marked with '.' do not appear in any of the integration branches, but I am
> still holding onto them.
> 
> Many topics that have been cooking since 1.7.1 pre-release freeze period
> are now in master; maybe it is a good time to start freezing feature
> branches for 1.7.2---I think I said I'll keep this cycle shorter, no?
> 
> --------------------------------------------------
> [New Topics]
> * mg/status-b (2010-05-25) 2 commits
>  - Documentation+t5708: document and test status -s -b
>  - Show branch information in short output of git status
> 
> There are a few style violations that snuck in, but otherwise looked
> sensible.

> --------------------------------------------------
> [Stalled -- would discard unless there are some movements soon]
> 
> * jc/rev-list-ancestry-path (2010-04-20) 1 commit
>  - revision: --ancestry-path
> 
> Just an illustration patch.  merge simplification logic used when
> pathspecs are in effect interacts with this rather badly.

> --------------------------------------------------
> [Cooking]
> 
> * em/checkout-orphan (2010-05-21) 5 commits
>  - bash completion: add --orphan to 'git checkout'
>  - t3200: test -l with core.logAllRefUpdates options
>  - checkout --orphan: respect -l option always
>  - refs: split log_ref_write logic into log_ref_setup
>  - Documentation: alter checkout --orphan description
> 
> In <4BFE2461.5080501@drmicha.warpmail.net>, Michael J Gruber raised a
> valid request for a better explanation of superfluous files left behind
> and then are cleaned.  Other than that I think this is a sane thing to
> do.

> * mg/rev-parse-lrbranches-locals (2010-05-14) 1 commit
>  - revlist: Introduce --lrbranches and --locals revision specifiers
>  (this branch uses mg/rev-parse-option-sifter-deprecation.)
> 
> Hmmm...

Now I know what to do ;)

[refalias]
	puextra	= origin/next..origin/pu
	punew	= origin/next@{1}..origin/next
	locals	= HEAD --branches --tags
	lrbranches = --branches --remotes

would make more sense and allows for personal choices. Hmmm? ;)

> 
> * mg/rev-parse-option-sifter-deprecation (2010-05-14) 3 commits
>  - t6018: make sure all tested symbolic names are different revs
>  - t6018: add tests for rev-list's --branches and --tags
>  - rev-parse: deprecate use as an option sifter
>  (this branch is used by mg/rev-parse-lrbranches-locals.)
> 
> Some people might interpret "Deprecate" too strongly; the intent is that
> we shouldn't keep piling parsing of new rev-list options to it and
> discourage the use of "option sifter" in new programs.

Jonathan Nieder· Jun 3, 2010, 18:25 UTC · re: Michael J Gruber · lore

mg/rev-parse-option-sifter-deprecation

Hi,

Michael J Gruber wrote:

> Junio C Hamano venit, vidit, dixit 03.06.2010 01:36:

>> Some people might interpret "Deprecate" too strongly; the intent is that
>> we shouldn't keep piling parsing of new rev-list options to it and
>> discourage the use of "option sifter" in new programs.
>
> So, s/deprecated/discouraged/? Is there an alternative we should suggest
> there for scripts needing to sift their options?

Ah, I was wondering about this, too.

 - teach setup_revisions() to use an option sifter instead of parsing
   options in one pass (probably a bad idea), or

 - give setup_revisions a second identity as an option sifter, or

 - teach filter-branch to do its own sifting of filespec arguments
   from revision arguments, and let rev-parse --revs-only rot

| You can use linkgit:git-rev-list[1] directly or
| +linkgit:git-for-each-ref[1] for scripting.

I do not understand the reference to for-each-ref here.

Clemens Buchacher· Jun 3, 2010, 13:36 UTC · re: Junio C Hamano · lore

[PATCH 1/2] separate quoting and relative path generation

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Wed, Jun 02, 2010 at 04:36:56PM -0700, Junio C Hamano wrote:

> * cb/ls-files-cdup (2010-05-26) 1 commit
>  - ls-files: allow relative pathspec
> 
> Doesn't write_name() quote twice when prefix_offset is non-zero?

Yes. Fixed in the following two patches.

 quote.c |   69 +++++++++++++++++++++++++++++++++++++++++---------------------
 quote.h |    8 ++++++-
 2 files changed, 52 insertions(+), 25 deletions(-)

diff --git a/quote.c b/quote.c
index fc93435..2ae2c1f 100644
--- a/quote.c
+++ b/quote.c
@@ -295,42 +295,63 @@ void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
-/* quote path as relative to the given prefix */
-char *quote_path_relative(const char *in, int len,
-			  struct strbuf *out, const char *prefix)
+void write_name_quoted_relative(const char *name, size_t len,
+				const char *prefix, size_t prefix_len,
+				FILE *fp, int terminator)
 {
-	int needquote;
+	struct strbuf sb = STRBUF_INIT;
 
+	char *path = path_relative(name, len, &sb, prefix, prefix_len);
+	write_name_quoted(path, fp, terminator);
+
+	strbuf_release(&sb);
+}
+
+/* give path as relative to prefix */
+char *path_relative(const char *in, int len,
+		    struct strbuf *out, const char *prefix, int prefix_len)
+{
 	if (len < 0)
 		len = strlen(in);
+	if (prefix && prefix_len < 0)
+		prefix_len = strlen(prefix);
 
-	/* "../" prefix itself does not need quoting, but "in" might. */
-	needquote = next_quote_pos(in, len) < len;
 	strbuf_setlen(out, 0);
 	strbuf_grow(out, len);
 
-	if (needquote)
-		strbuf_addch(out, '"');
-	if (prefix) {
-		int off = 0;
-		while (prefix[off] && off < len && prefix[off] == in[off])
-			if (prefix[off] == '/') {
-				prefix += off + 1;
-				in += off + 1;
-				len -= off + 1;
-				off = 0;
-			} else
-				off++;
-
-		for (; *prefix; prefix++)
-			if (*prefix == '/')
+	if (prefix_len > 0) {
+		int off = 0, i = 0;
+		while (i < prefix_len && i < len && prefix[i] == in[i]) {
+			if (prefix[i] == '/')
+				off = i + 1;
+			i++;
+		}
+		in += off;
+		len -= off;
+
+		while (i < prefix_len) {
+			if (prefix[i] == '/')
 				strbuf_addstr(out, "../");
+			i++;
+		}
 	}
+	strbuf_add(out, in, len);
+
+	return out->buf;
+}
+
+/* quote path as relative to the given prefix */
+char *quote_path_relative(const char *in, int len,
+			  struct strbuf *out, const char *prefix)
+{
+	char *rel;
+	size_t rel_len;
 
-	quote_c_style_counted (in, len, out, NULL, 1);
+	path_relative(in, len, out, prefix, -1);
+	rel = strbuf_detach(out, &rel_len);
+	quote_c_style_counted(rel, rel_len, out, NULL, 0);
+	free(rel);
 
-	if (needquote)
-		strbuf_addch(out, '"');
 	if (!out->len)
 		strbuf_addstr(out, "./");
 
diff --git a/quote.h b/quote.h
index f83eb23..e9e0221 100644
--- a/quote.h
+++ b/quote.h
@@ -54,9 +54,15 @@ extern void quote_two_c_style(struct strbuf *, const char *, const char *, int);
 extern void write_name_quoted(const char *name, FILE *, int terminator);
 extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
                                  const char *name, FILE *, int terminator);
+extern void write_name_quoted_relative(const char *name, size_t len,
+		const char *prefix, size_t prefix_len,
+		FILE *fp, int terminator);
 
+/* give path as relative to prefix */
+extern char *path_relative(const char *in, int len,
+		struct strbuf *out, const char *prefix, int prefix_len);
 /* quote path as relative to the given prefix */
-char *quote_path_relative(const char *in, int len,
+extern char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix);
 
 /* quoting as a string literal for other languages */

-- 
1.7.1.2.g2a651

Clemens Buchacher· Jun 3, 2010, 13:39 UTC · re: Clemens Buchacher · lore

[PATCH 2/2] ls-files: allow relative pathspec

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

- write_name() now uses the new write_name_quoted_relative()
- prefix_offset is renamed to prefix_len, which is much more
  intuitive

Clemens

 builtin/ls-files.c |   75 ++++++++++++++++++++++++++++-----------------------
 t/t7010-setup.sh   |   12 +++-----
 2 files changed, 45 insertions(+), 42 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index c0fbcdc..0804047 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -26,8 +26,9 @@ static int show_killed;
 static int show_valid_bit;
 static int line_terminator = '\n';
 
+static const char *prefix;
+static int max_prefix_len;
 static int prefix_len;
-static int prefix_offset;
 static const char **pathspec;
 static int error_unmatch;
 static char *ps_matched;
@@ -43,10 +44,15 @@ static const char *tag_modified = "";
 static const char *tag_skip_worktree = "";
 static const char *tag_resolve_undo = "";
 
+static void write_name(const char* name, size_t len)
+{
+	write_name_quoted_relative(name, len, prefix, prefix_len, stdout,
+			line_terminator);
+}
+
 static void show_dir_entry(const char *tag, struct dir_entry *ent)
 {
-	int len = prefix_len;
-	int offset = prefix_offset;
+	int len = max_prefix_len;
 
 	if (len >= ent->len)
 		die("git ls-files: internal error - directory entry not superset of prefix");
@@ -55,7 +61,7 @@ static void show_dir_entry(const char *tag, struct dir_entry *ent)
 		return;
 
 	fputs(tag, stdout);
-	write_name_quoted(ent->name + offset, stdout, line_terminator);
+	write_name(ent->name, ent->len);
 }
 
 static void show_other_files(struct dir_struct *dir)
@@ -121,8 +127,7 @@ static void show_killed_files(struct dir_struct *dir)
 
 static void show_ce_entry(const char *tag, struct cache_entry *ce)
 {
-	int len = prefix_len;
-	int offset = prefix_offset;
+	int len = max_prefix_len;
 
 	if (len >= ce_namelen(ce))
 		die("git ls-files: internal error - cache entry not superset of prefix");
@@ -156,20 +161,19 @@ static void show_ce_entry(const char *tag, struct cache_entry *ce)
 		       find_unique_abbrev(ce->sha1,abbrev),
 		       ce_stage(ce));
 	}
-	write_name_quoted(ce->name + offset, stdout, line_terminator);
+	write_name(ce->name, ce_namelen(ce));
 }
 
 static int show_one_ru(struct string_list_item *item, void *cbdata)
 {
-	int offset = prefix_offset;
 	const char *path = item->string;
 	struct resolve_undo_info *ui = item->util;
 	int i, len;
 
 	len = strlen(path);
-	if (len < prefix_len)
+	if (len < max_prefix_len)
 		return 0; /* outside of the prefix */
-	if (!match_pathspec(pathspec, path, len, prefix_len, ps_matched))
+	if (!match_pathspec(pathspec, path, len, max_prefix_len, ps_matched))
 		return 0; /* uninterested */
 	for (i = 0; i < 3; i++) {
 		if (!ui->mode[i])
@@ -177,19 +181,19 @@ static int show_one_ru(struct string_list_item *item, void *cbdata)
 		printf("%s%06o %s %d\t", tag_resolve_undo, ui->mode[i],
 		       find_unique_abbrev(ui->sha1[i], abbrev),
 		       i + 1);
-		write_name_quoted(path + offset, stdout, line_terminator);
+		write_name(path, len);
 	}
 	return 0;
 }
 
-static void show_ru_info(const char *prefix)
+static void show_ru_info(void)
 {
 	if (!the_index.resolve_undo)
 		return;
 	for_each_string_list(show_one_ru, the_index.resolve_undo, NULL);
 }
 
-static void show_files(struct dir_struct *dir, const char *prefix)
+static void show_files(struct dir_struct *dir)
 {
 	int i;
 
@@ -243,7 +247,7 @@ static void show_files(struct dir_struct *dir, const char *prefix)
  */
 static void prune_cache(const char *prefix)
 {
-	int pos = cache_name_pos(prefix, prefix_len);
+	int pos = cache_name_pos(prefix, max_prefix_len);
 	unsigned int first, last;
 
 	if (pos < 0)
@@ -256,7 +260,7 @@ static void prune_cache(const char *prefix)
 	while (last > first) {
 		int next = (last + first) >> 1;
 		struct cache_entry *ce = active_cache[next];
-		if (!strncmp(ce->name, prefix, prefix_len)) {
+		if (!strncmp(ce->name, prefix, max_prefix_len)) {
 			first = next+1;
 			continue;
 		}
@@ -265,11 +269,16 @@ static void prune_cache(const char *prefix)
 	active_nr = last;
 }
 
-static const char *verify_pathspec(const char *prefix)
+static const char *pathspec_prefix(const char *prefix)
 {
 	const char **p, *n, *prev;
 	unsigned long max;
 
+	if (!pathspec) {
+		max_prefix_len = prefix ? strlen(prefix) : 0;
+		return prefix;
+	}
+
 	prev = NULL;
 	max = PATH_MAX;
 	for (p = pathspec; (n = *p) != NULL; p++) {
@@ -291,10 +300,7 @@ static const char *verify_pathspec(const char *prefix)
 		}
 	}
 
-	if (prefix_offset > max || memcmp(prev, prefix, prefix_offset))
-		die("git ls-files: cannot generate relative filenames containing '..'");
-
-	prefix_len = max;
+	max_prefix_len = max;
 	return max ? xmemdupz(prev, max) : NULL;
 }
 
@@ -374,7 +380,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 	}
 }
 
-int report_path_error(const char *ps_matched, const char **pathspec, int prefix_offset)
+int report_path_error(const char *ps_matched, const char **pathspec, int prefix_len)
 {
 	/*
 	 * Make sure all pathspec matched; otherwise it is an error.
@@ -404,7 +410,7 @@ int report_path_error(const char *ps_matched, const char **pathspec, int prefix_
 			continue;
 
 		error("pathspec '%s' did not match any file(s) known to git.",
-		      pathspec[num] + prefix_offset);
+		      pathspec[num] + prefix_len);
 		errors++;
 	}
 	return errors;
@@ -456,9 +462,10 @@ static int option_parse_exclude_standard(const struct option *opt,
 	return 0;
 }
 
-int cmd_ls_files(int argc, const char **argv, const char *prefix)
+int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 {
 	int require_work_tree = 0, show_tag = 0;
+	const char *max_prefix;
 	struct dir_struct dir;
 	struct option builtin_ls_files_options[] = {
 		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
@@ -504,7 +511,7 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK, 0, "exclude-standard", &dir, NULL,
 			"add the standard git exclusions",
 			PARSE_OPT_NOARG, option_parse_exclude_standard },
-		{ OPTION_SET_INT, 0, "full-name", &prefix_offset, NULL,
+		{ OPTION_SET_INT, 0, "full-name", &prefix_len, NULL,
 			"make the output relative to the project top directory",
 			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL },
 		OPT_BOOLEAN(0, "error-unmatch", &error_unmatch,
@@ -516,8 +523,9 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	};
 
 	memset(&dir, 0, sizeof(dir));
+	prefix = cmd_prefix;
 	if (prefix)
-		prefix_offset = strlen(prefix);
+		prefix_len = strlen(prefix);
 	git_config(git_default_config, NULL);
 
 	if (read_cache() < 0)
@@ -555,9 +563,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	if (pathspec)
 		strip_trailing_slash_from_submodules();
 
-	/* Verify that the pathspec matches the prefix */
-	if (pathspec)
-		prefix = verify_pathspec(prefix);
+	/* Find common prefix for all pathspec's */
+	max_prefix = pathspec_prefix(prefix);
 
 	/* Treat unmatching pathspec elements as errors */
 	if (pathspec && error_unmatch) {
@@ -575,8 +582,8 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 	      show_killed | show_modified | show_resolve_undo))
 		show_cached = 1;
 
-	if (prefix)
-		prune_cache(prefix);
+	if (max_prefix)
+		prune_cache(max_prefix);
 	if (with_tree) {
 		/*
 		 * Basic sanity check; show-stages and show-unmerged
@@ -584,15 +591,15 @@ int cmd_ls_files(int argc, const char **argv, const char *prefix)
 		 */
 		if (show_stage || show_unmerged)
 			die("ls-files --with-tree is incompatible with -s or -u");
-		overlay_tree_on_cache(with_tree, prefix);
+		overlay_tree_on_cache(with_tree, max_prefix);
 	}
-	show_files(&dir, prefix);
+	show_files(&dir);
 	if (show_resolve_undo)
-		show_ru_info(prefix);
+		show_ru_info();
 
 	if (ps_matched) {
 		int bad;
-		bad = report_path_error(ps_matched, pathspec, prefix_offset);
+		bad = report_path_error(ps_matched, pathspec, prefix_len);
 		if (bad)
 			fprintf(stderr, "Did you forget to 'git add'?\n");
 
diff --git a/t/t7010-setup.sh b/t/t7010-setup.sh
index d8a7c79..0335a9a 100755
--- a/t/t7010-setup.sh
+++ b/t/t7010-setup.sh
@@ -103,14 +103,10 @@ test_expect_success 'git ls-files (relative #3)' '
 	git add a &&
 	(
 		cd a/b &&
-		if git ls-files "../e/f"
-		then
-			echo Gaah, should have failed
-			exit 1
-		else
-			: happy
-		fi
-	)
+		git ls-files "../e/f"
+	)  >current &&
+	echo ../e/f >expect &&
+	test_cmp expect current
 
 '
 

-- 
1.7.1.2.g2a651

Junio C Hamano· Jun 3, 2010, 22:16 UTC · re: Clemens Buchacher · lore

Re: [PATCH 1/2] separate quoting and relative path generation

Clemens Buchacher <drizzd@aon.at> writes:

> +/* give path as relative to prefix */
> +char *path_relative(const char *in, int len,
> +		    struct strbuf *out, const char *prefix, int prefix_len)
> +{
>  	if (len < 0)
>  		len = strlen(in);
> +	if (prefix && prefix_len < 0)
> +		prefix_len = strlen(prefix);
>  
>  	strbuf_setlen(out, 0);
>  	strbuf_grow(out, len);
>  
> +	if (prefix_len > 0) {
> +		int off = 0, i = 0;
> +		while (i < prefix_len && i < len && prefix[i] == in[i]) {
> +			if (prefix[i] == '/')
> +				off = i + 1;
> +			i++;
> +		}
> +		in += off;
> +		len -= off;
> +
> +		while (i < prefix_len) {
> +			if (prefix[i] == '/')
>  				strbuf_addstr(out, "../");
> +			i++;
> +		}
>  	}
> +	strbuf_add(out, in, len);
> +
> +	return out->buf;
> +}

Clemens Buchacher· Jun 4, 2010, 07:44 UTC · re: Junio C Hamano · lore

[PATCH] optimize path_relative()

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Thu, Jun 03, 2010 at 03:16:33PM -0700, Junio C Hamano wrote:

> Clemens Buchacher <drizzd@aon.at> writes:
>
> > +	strbuf_add(out, in, len);
> > +
> > +	return out->buf;
> > +}
> 
> Hmm...  I wonder if we really want to always make a copy of the string in
> the majority of the case where there is no need to add ../ and the path
> does not have any funny characters that needs quoting.

Please let me know if you'd rather have a resend.

Clemens

 quote.c |   72 ++++++++++++++++++++++++++++++++++++--------------------------
 quote.h |    3 --
 2 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/quote.c b/quote.c
index 2ae2c1f..481541a 100644
--- a/quote.c
+++ b/quote.c
@@ -295,62 +295,74 @@ void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len);
+
 void write_name_quoted_relative(const char *name, size_t len,
 				const char *prefix, size_t prefix_len,
 				FILE *fp, int terminator)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	char *path = path_relative(name, len, &sb, prefix, prefix_len);
-	write_name_quoted(path, fp, terminator);
+	name = path_relative(name, len, &sb, prefix, prefix_len);
+	write_name_quoted(name, fp, terminator);
 
 	strbuf_release(&sb);
 }
 
-/* give path as relative to prefix */
-char *path_relative(const char *in, int len,
-		    struct strbuf *out, const char *prefix, int prefix_len)
+/*
+ * Give path as relative to prefix.
+ *
+ * The strbuf may or may not be used, so do not assume it contains the
+ * returned path.
+ */
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len)
 {
+	int off, i;
+
 	if (len < 0)
 		len = strlen(in);
 	if (prefix && prefix_len < 0)
 		prefix_len = strlen(prefix);
 
-	strbuf_setlen(out, 0);
-	strbuf_grow(out, len);
+	off = 0;
+	i = 0;
+	while (i < prefix_len && i < len && prefix[i] == in[i]) {
+		if (prefix[i] == '/')
+			off = i + 1;
+		i++;
+	}
+	in += off;
+	len -= off;
 
-	if (prefix_len > 0) {
-		int off = 0, i = 0;
-		while (i < prefix_len && i < len && prefix[i] == in[i]) {
-			if (prefix[i] == '/')
-				off = i + 1;
-			i++;
-		}
-		in += off;
-		len -= off;
+	if (i == prefix_len)
+		return in;
 
-		while (i < prefix_len) {
-			if (prefix[i] == '/')
-				strbuf_addstr(out, "../");
-			i++;
-		}
+	strbuf_reset(sb);
+	strbuf_grow(sb, len);
+
+	while (i < prefix_len) {
+		if (prefix[i] == '/')
+			strbuf_addstr(sb, "../");
+		i++;
 	}
-	strbuf_add(out, in, len);
+	strbuf_add(sb, in, len);
 
-	return out->buf;
+	return sb->buf;
 }
 
 /* quote path as relative to the given prefix */
 char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix)
 {
-	char *rel;
-	size_t rel_len;
-
-	path_relative(in, len, out, prefix, -1);
-	rel = strbuf_detach(out, &rel_len);
-	quote_c_style_counted(rel, rel_len, out, NULL, 0);
-	free(rel);
+	struct strbuf sb = STRBUF_INIT;
+	const char *rel = path_relative(in, len, &sb, prefix, -1);
+	strbuf_reset(out);
+	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
+	strbuf_release(&sb);
 
 	if (!out->len)
 		strbuf_addstr(out, "./");
diff --git a/quote.h b/quote.h
index e9e0221..38003bf 100644
--- a/quote.h
+++ b/quote.h
@@ -58,9 +58,6 @@ extern void write_name_quoted_relative(const char *name, size_t len,
 		const char *prefix, size_t prefix_len,
 		FILE *fp, int terminator);
 
-/* give path as relative to prefix */
-extern char *path_relative(const char *in, int len,
-		struct strbuf *out, const char *prefix, int prefix_len);
 /* quote path as relative to the given prefix */
 extern char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix);

-- 
1.7.1.2.ga1f6e

Clemens Buchacher· Jun 4, 2010, 07:50 UTC · re: Clemens Buchacher · lore

Re: [PATCH] optimize path_relative()

On Fri, Jun 04, 2010 at 09:44:43AM +0200, Clemens Buchacher wrote:

> +static const char *path_relative(const char *in, int len,
> +				 struct strbuf *sb, const char *prefix,
> +				 int prefix_len)
>  {
> +	int off, i;
> +
>  	if (len < 0)
>  		len = strlen(in);
>  	if (prefix && prefix_len < 0)
>  		prefix_len = strlen(prefix);
>  
> -	strbuf_setlen(out, 0);
> -	strbuf_grow(out, len);
> +	off = 0;
> +	i = 0;
> +	while (i < prefix_len && i < len && prefix[i] == in[i]) {
> +		if (prefix[i] == '/')
> +			off = i + 1;
> +		i++;
> +	}
> +	in += off;
> +	len -= off;
>  
> -	if (prefix_len > 0) {
> -		int off = 0, i = 0;
> -		while (i < prefix_len && i < len && prefix[i] == in[i]) {
> -			if (prefix[i] == '/')
> -				off = i + 1;
> -			i++;
> -		}
> -		in += off;
> -		len -= off;
> +	if (i == prefix_len)
> +		return in;
>  
> -		while (i < prefix_len) {
> -			if (prefix[i] == '/')
> -				strbuf_addstr(out, "../");
> -			i++;
> -		}
> +	strbuf_reset(sb);
> +	strbuf_grow(sb, len);
> +
> +	while (i < prefix_len) {
> +		if (prefix[i] == '/')
> +			strbuf_addstr(sb, "../");
> +		i++;
>  	}

> -	strbuf_add(out, in, len);
> +	strbuf_add(sb, in, len);
>  
> -	return out->buf;
> +	return sb->buf;
>  }

Clemens Buchacher· Jun 5, 2010, 08:04 UTC · re: Clemens Buchacher · lore

[PATCH] setup: document prefix

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Fri, Jun 04, 2010 at 09:50:34AM +0200, Clemens Buchacher wrote:

>
> By the way, I noticed that we rely on the fact that a non-empty
> prefix ends with '/'. Is that ok?

 setup.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/setup.c b/setup.c
index 5716d90..0e4cfe6 100644
--- a/setup.c
+++ b/setup.c
@@ -519,6 +519,12 @@ int check_repository_format(void)
 	return check_repository_format_gently(NULL);
 }
 
+/*
+ * Returns the "prefix", a path to the current working directory
+ * relative to the work tree root, or NULL, if the current working
+ * directory is not a strict subdirectory of the work tree root. The
+ * prefix always ends with a '/' character.
+ */
 const char *setup_git_directory(void)
 {
 	const char *retval = setup_git_directory_gently(NULL);

-- 
1.7.1.2.ga1f6e

Clemens Buchacher· Jun 5, 2010, 07:37 UTC · re: Clemens Buchacher · lore

[PATCH v2] optimize path_relative()

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

On Fri, Jun 04, 2010 at 09:44:43AM +0200, Clemens Buchacher wrote:

>  	if (prefix && prefix_len < 0)
>  		prefix_len = strlen(prefix);

[...]

> +	if (i == prefix_len)
> +		return in;

 quote.c |   72 ++++++++++++++++++++++++++++++++++++--------------------------
 quote.h |    3 --
 2 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/quote.c b/quote.c
index 2ae2c1f..63d3b01 100644
--- a/quote.c
+++ b/quote.c
@@ -295,62 +295,74 @@ void write_name_quotedpfx(const char *pfx, size_t pfxlen,
 	fputc(terminator, fp);
 }
 
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len);
+
 void write_name_quoted_relative(const char *name, size_t len,
 				const char *prefix, size_t prefix_len,
 				FILE *fp, int terminator)
 {
 	struct strbuf sb = STRBUF_INIT;
 
-	char *path = path_relative(name, len, &sb, prefix, prefix_len);
-	write_name_quoted(path, fp, terminator);
+	name = path_relative(name, len, &sb, prefix, prefix_len);
+	write_name_quoted(name, fp, terminator);
 
 	strbuf_release(&sb);
 }
 
-/* give path as relative to prefix */
-char *path_relative(const char *in, int len,
-		    struct strbuf *out, const char *prefix, int prefix_len)
+/*
+ * Give path as relative to prefix.
+ *
+ * The strbuf may or may not be used, so do not assume it contains the
+ * returned path.
+ */
+static const char *path_relative(const char *in, int len,
+				 struct strbuf *sb, const char *prefix,
+				 int prefix_len)
 {
+	int off, i;
+
 	if (len < 0)
 		len = strlen(in);
 	if (prefix && prefix_len < 0)
 		prefix_len = strlen(prefix);
 
-	strbuf_setlen(out, 0);
-	strbuf_grow(out, len);
+	off = 0;
+	i = 0;
+	while (i < prefix_len && i < len && prefix[i] == in[i]) {
+		if (prefix[i] == '/')
+			off = i + 1;
+		i++;
+	}
+	in += off;
+	len -= off;
 
-	if (prefix_len > 0) {
-		int off = 0, i = 0;
-		while (i < prefix_len && i < len && prefix[i] == in[i]) {
-			if (prefix[i] == '/')
-				off = i + 1;
-			i++;
-		}
-		in += off;
-		len -= off;
+	if (i >= prefix_len)
+		return in;
 
-		while (i < prefix_len) {
-			if (prefix[i] == '/')
-				strbuf_addstr(out, "../");
-			i++;
-		}
+	strbuf_reset(sb);
+	strbuf_grow(sb, len);
+
+	while (i < prefix_len) {
+		if (prefix[i] == '/')
+			strbuf_addstr(sb, "../");
+		i++;
 	}
-	strbuf_add(out, in, len);
+	strbuf_add(sb, in, len);
 
-	return out->buf;
+	return sb->buf;
 }
 
 /* quote path as relative to the given prefix */
 char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix)
 {
-	char *rel;
-	size_t rel_len;
-
-	path_relative(in, len, out, prefix, -1);
-	rel = strbuf_detach(out, &rel_len);
-	quote_c_style_counted(rel, rel_len, out, NULL, 0);
-	free(rel);
+	struct strbuf sb = STRBUF_INIT;
+	const char *rel = path_relative(in, len, &sb, prefix, -1);
+	strbuf_reset(out);
+	quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
+	strbuf_release(&sb);
 
 	if (!out->len)
 		strbuf_addstr(out, "./");
diff --git a/quote.h b/quote.h
index e9e0221..38003bf 100644
--- a/quote.h
+++ b/quote.h
@@ -58,9 +58,6 @@ extern void write_name_quoted_relative(const char *name, size_t len,
 		const char *prefix, size_t prefix_len,
 		FILE *fp, int terminator);
 
-/* give path as relative to prefix */
-extern char *path_relative(const char *in, int len,
-		struct strbuf *out, const char *prefix, int prefix_len);
 /* quote path as relative to the given prefix */
 extern char *quote_path_relative(const char *in, int len,
 			  struct strbuf *out, const char *prefix);

-- 
1.7.1.2.ga1f6e

Junio C Hamano· Jun 5, 2010, 18:07 UTC · re: Clemens Buchacher · lore

Re: [PATCH v2] optimize path_relative()

Clemens Buchacher <drizzd@aon.at> writes:

> Avoid copying to strbuf in case a subset of the original string can
> be returned.
>
> Since the strbuf is no longer guaranteed to be updated, this
> function is different from quote_path_relative(). To avoid
> confusion, do not export it.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>

Thanks.

Thomas Rast· Jun 3, 2010, 14:36 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

Junio C Hamano wrote:

> * ld/discovery-limit-to-fs (2010-04-04) 1 commit
>  - write-index: check and warn when worktree crosses a filesystem boundary
> 
> There might have been some valid objections to this but I cannot recall.
> Will merge to 'next' unless I hear something within a few days.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Eyvind Bernhardsen· Jun 3, 2010, 19:53 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

Hi,

On 3. juni 2010, at 01.36, Junio C Hamano wrote:

> * eb/core-eol (2010-05-19) 4 commits
> - Add "core.eol" config variable
> - Rename the "crlf" attribute "text"
> - Add per-repository eol normalization
> - Add tests for per-repository eol normalization
> (this branch uses fg/autocrlf.)
> 
> Will merge to 'next'.

I have a fixup for the "core.eol" commit that modifies "core.autocrlf"'s behaviour and fixes the documentation, as discussed on the list.  Unfortunately it's on a machine that's not accessible to me right now, but I'll send it tomorrow.

-- 
Eyvind

Jonathan Nieder· Jun 4, 2010, 21:18 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

Junio C Hamano wrote:

> * gv/portable (2010-05-14) 18 commits

[...]

>  - Do not use "diff" found on PATH while building and installing

Maybe it could be made portable with ‘git diff --no-index’?

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 3145009..d067894 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -107,7 +107,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 		# remove lines that are unique to ours.
 		orig=`git-unpack-file $2`
 		sz0=`wc -c <"$orig"`
-		$DIFF -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
+		diff -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
 		sz1=`wc -c <"$orig"`
 
 		# If we do not have enough common material, it is not

Junio C Hamano· Jun 5, 2010, 18:07 UTC · re: Jonathan Nieder · lore

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>
>> * gv/portable (2010-05-14) 18 commits
> [...]
>>  - Do not use "diff" found on PATH while building and installing
>
> That patch (d1b1a919) breaks merge-one-file when run outside the test
> suite as far as I can tell.

 Makefile              |    2 ++
 git-merge-one-file.sh |    2 +-
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index bcb84be..0367e8a 100644
--- a/Makefile
+++ b/Makefile
@@ -1494,6 +1494,7 @@ SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
 PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
 PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
 TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
+DIFF_SQ = $(subst ','\'',$(DIFF))
 
 LIBS = $(GITLIBS) $(EXTLIBS)
 
@@ -1582,6 +1583,7 @@ define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
     -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
+    -e 's|@@DIFF@@|$(DIFF_SQ)|' \
     -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
     -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
     -e $(BROKEN_PATH_FIX) \
diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 3145009..b86402a 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -107,7 +107,7 @@ case "${1:-.}${2:-.}${3:-.}" in
 		# remove lines that are unique to ours.
 		orig=`git-unpack-file $2`
 		sz0=`wc -c <"$orig"`
-		$DIFF -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
+		@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
 		sz1=`wc -c <"$orig"`
 
 		# If we do not have enough common material, it is not

Jonathan Nieder· Jun 5, 2010, 19:32 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

Jun 05, 2010 at 11:07:32AM -0700, Junio C Hamano wrote:

> @@ -1582,6 +1583,7 @@ define cmd_munge_script
>  $(RM) $@ $@+ && \
>  sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>      -e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
> +    -e 's|@@DIFF@@|$(DIFF_SQ)|' \
>      -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
>      -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
>      -e $(BROKEN_PATH_FIX) \
> diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
> index 3145009..b86402a 100755
> --- a/git-merge-one-file.sh
> +++ b/git-merge-one-file.sh
> @@ -107,7 +107,7 @@ case "${1:-.}${2:-.}${3:-.}" in
>  		# remove lines that are unique to ours.
>  		orig=`git-unpack-file $2`
>  		sz0=`wc -c <"$orig"`
> -		$DIFF -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
> +		@@DIFF@@ -u -La/$orig -Lb/$orig $orig $src2 | git apply --no-add
>  		sz1=`wc -c <"$orig"`
>  
>  		# If we do not have enough common material, it is not

Looks good to me.  Thanks.

Sverre Rabbelier· Jun 5, 2010, 23:57 UTC · re: Junio C Hamano · lore

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

Heya,

On Sat, Jun 5, 2010 at 20:07, Junio C Hamano <gitster@pobox.com> wrote:

>> That patch (d1b1a919) breaks merge-one-file when run outside the test
>> suite as far as I can tell.
>
> Thanks for catching.  I wonder if something like this would be better in
> that it wouldn't break people who do not use custom "DIFF" while it would
> help people who do at the same time...
>
>  Makefile              |    2 ++
>  git-merge-one-file.sh |    2 +-

No addition to the test suite to catch this in the future?

-- 
Cheers,

Sverre Rabbelier

Junio C Hamano· Jun 6, 2010, 04:00 UTC · re: Sverre Rabbelier · lore

Re: What's cooking in git.git (Jun 2010, #01; Wed, 2)

Sverre Rabbelier <srabbelier@gmail.com> writes:

> No addition to the test suite to catch this in the future?

If we had a test like this:

    test_expect_success 'run resolve' '
	... set up something to merge ...
        (
        	unset DIFF;
                git merge -s resolve ...
	)
    '

it would have caught this particular breakage, but

 (1) nobody could have anticipated before the problematic patch that $DIFF
     would be an exported variable that could cause trouble, so nobody
     could have written it before the $DIFF patch;

 (2) with the knowledge necessary to write the above test, the $DIFF patch
     wouldn't have made the misconversion, so it is unreasonable to say
     that the $DIFF patch should have come with the above test; and

 (3) having the above test in the test suite may catch exactly the same
     mistake in git-merge-one-file.sh, but that is not a guarantee that
     nobody will break another scripted Porcelain that is not triggered in
     the "resolve" codepath.