* [PATCH 3/5] Allow alternate "low-level" emit function from xdl_diff
From: René Scharfe @ 2008-10-25 13:30 UTC (permalink / raw)
To: Brian Downing; +Cc: Junio C Hamano, git
In-Reply-To: <48BF0FBF.3010104@lsrfire.ath.cx>
From: Brian Downing <bdowning@lavos.net>
For some users (e.g. git blame), getting textual patch output is just
extra work, as they can get all the information they need from the low-
level diff structures. Allow for an alternate low-level emit function
to be defined to allow bypassing the textual patch generation; set
xemitconf_t's emit_func member to enable this.
The (void (*)()) type is pretty ugly, but the alternative would be to
include most of the private xdiff headers in xdiff.h to get the types
required for the "proper" function prototype. Also, a (void *) won't
work, as ANSI C doesn't allow a function pointer to be cast to an
object pointer.
Signed-off-by: Brian Downing <bdowning@lavos.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Taken from pu.
xdiff/xdiff.h | 1 +
xdiff/xdiffi.c | 4 +++-
xdiff/xemit.c | 3 +--
xdiff/xemit.h | 3 +++
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/xdiff/xdiff.h b/xdiff/xdiff.h
index deebe02..84fff58 100644
--- a/xdiff/xdiff.h
+++ b/xdiff/xdiff.h
@@ -87,6 +87,7 @@ typedef struct s_xdemitconf {
unsigned long flags;
find_func_t find_func;
void *find_func_priv;
+ void (*emit_func)();
} xdemitconf_t;
typedef struct s_bdiffparam {
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 1bad846..9d0324a 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -538,6 +538,8 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
xdemitconf_t const *xecfg, xdemitcb_t *ecb) {
xdchange_t *xscr;
xdfenv_t xe;
+ emit_func_t ef = xecfg->emit_func ?
+ (emit_func_t)xecfg->emit_func : xdl_emit_diff;
if (xdl_do_diff(mf1, mf2, xpp, &xe) < 0) {
@@ -551,7 +553,7 @@ int xdl_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp,
return -1;
}
if (xscr) {
- if (xdl_emit_diff(&xe, xscr, ecb, xecfg) < 0) {
+ if (ef(&xe, xscr, ecb, xecfg) < 0) {
xdl_free_script(xscr);
xdl_free_env(&xe);
diff --git a/xdiff/xemit.c b/xdiff/xemit.c
index d3d9c84..4625c1b 100644
--- a/xdiff/xemit.c
+++ b/xdiff/xemit.c
@@ -27,7 +27,6 @@
static long xdl_get_rec(xdfile_t *xdf, long ri, char const **rec);
static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *ecb);
-static xdchange_t *xdl_get_hunk(xdchange_t *xscr, xdemitconf_t const *xecfg);
@@ -58,7 +57,7 @@ static int xdl_emit_record(xdfile_t *xdf, long ri, char const *pre, xdemitcb_t *
* Starting at the passed change atom, find the latest change atom to be included
* inside the differential hunk according to the specified configuration.
*/
-static xdchange_t *xdl_get_hunk(xdchange_t *xscr, xdemitconf_t const *xecfg) {
+xdchange_t *xdl_get_hunk(xdchange_t *xscr, xdemitconf_t const *xecfg) {
xdchange_t *xch, *xchp;
for (xchp = xscr, xch = xscr->next; xch; xchp = xch, xch = xch->next)
diff --git a/xdiff/xemit.h b/xdiff/xemit.h
index 440a739..c2e2e83 100644
--- a/xdiff/xemit.h
+++ b/xdiff/xemit.h
@@ -24,7 +24,10 @@
#define XEMIT_H
+typedef int (*emit_func_t)(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
+ xdemitconf_t const *xecfg);
+xdchange_t *xdl_get_hunk(xdchange_t *xscr, xdemitconf_t const *xecfg);
int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb,
xdemitconf_t const *xecfg);
--
1.6.0.3.514.g2f91b
^ permalink raw reply related
* [PATCH 2/5] Always initialize xpparam_t to 0
From: René Scharfe @ 2008-10-25 13:30 UTC (permalink / raw)
To: Brian Downing; +Cc: Junio C Hamano, git
In-Reply-To: <48BF0FBF.3010104@lsrfire.ath.cx>
From: Brian Downing <bdowning@lavos.net>
We're going to be adding some parameters to this, so we can't have
any uninitialized data in it.
Signed-off-by: Brian Downing <bdowning@lavos.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Taken from pu. This patch series doesn't add anything to the struct,
but it's a good idea to future-proof its initialization anyway.
builtin-blame.c | 1 +
builtin-rerere.c | 1 +
combine-diff.c | 1 +
diff.c | 5 +++++
merge-file.c | 1 +
5 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/builtin-blame.c b/builtin-blame.c
index 593b539..5ca7065 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -523,6 +523,7 @@ static struct patch *compare_buffer(mmfile_t
*file_p, mmfile_t *file_o,
xdemitconf_t xecfg;
xdemitcb_t ecb;
+ memset(&xpp, 0, sizeof(xpp));
xpp.flags = xdl_opts;
memset(&xecfg, 0, sizeof(xecfg));
xecfg.ctxlen = context;
diff --git a/builtin-rerere.c b/builtin-rerere.c
index dd4573f..d4dec6b 100644
--- a/builtin-rerere.c
+++ b/builtin-rerere.c
@@ -98,6 +98,7 @@ static int diff_two(const char *file1, const char *label1,
printf("--- a/%s\n+++ b/%s\n", label1, label2);
fflush(stdout);
+ memset(&xpp, 0, sizeof(xpp));
xpp.flags = XDF_NEED_MINIMAL;
memset(&xecfg, 0, sizeof(xecfg));
xecfg.ctxlen = 3;
diff --git a/combine-diff.c b/combine-diff.c
index 5aa1104..ec8df39 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -213,6 +213,7 @@ static void combine_diff(const unsigned char
*parent, mmfile_t *result_file,
parent_file.ptr = grab_blob(parent, &sz);
parent_file.size = sz;
+ memset(&xpp, 0, sizeof(xpp));
xpp.flags = XDF_NEED_MINIMAL;
memset(&xecfg, 0, sizeof(xecfg));
memset(&state, 0, sizeof(state));
diff --git a/diff.c b/diff.c
index e368fef..1918b73 100644
--- a/diff.c
+++ b/diff.c
@@ -400,6 +400,7 @@ static void diff_words_show(struct diff_words_data
*diff_words)
mmfile_t minus, plus;
int i;
+ memset(&xpp, 0, sizeof(xpp));
memset(&xecfg, 0, sizeof(xecfg));
minus.size = diff_words->minus.text.size;
minus.ptr = xmalloc(minus.size);
@@ -1416,6 +1417,7 @@ static void builtin_diff(const char *name_a,
if (!pe)
pe = diff_funcname_pattern(two);
+ memset(&xpp, 0, sizeof(xpp));
memset(&xecfg, 0, sizeof(xecfg));
memset(&ecbdata, 0, sizeof(ecbdata));
ecbdata.label_path = lbl;
@@ -1489,6 +1491,7 @@ static void builtin_diffstat(const char *name_a,
const char *name_b,
xdemitconf_t xecfg;
xdemitcb_t ecb;
+ memset(&xpp, 0, sizeof(xpp));
memset(&xecfg, 0, sizeof(xecfg));
xpp.flags = XDF_NEED_MINIMAL | o->xdl_opts;
xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
@@ -1535,6 +1538,7 @@ static void builtin_checkdiff(const char *name_a,
const char *name_b,
xdemitconf_t xecfg;
xdemitcb_t ecb;
+ memset(&xpp, 0, sizeof(xpp));
memset(&xecfg, 0, sizeof(xecfg));
xecfg.ctxlen = 1; /* at least one context line */
xpp.flags = XDF_NEED_MINIMAL;
@@ -2958,6 +2962,7 @@ static int diff_get_patch_id(struct diff_options
*options, unsigned char *sha1)
struct diff_filepair *p = q->queue[i];
int len1, len2;
+ memset(&xpp, 0, sizeof(xpp));
memset(&xecfg, 0, sizeof(xecfg));
if (p->status == 0)
return error("internal diff status error");
diff --git a/merge-file.c b/merge-file.c
index 2a939c9..3120a95 100644
--- a/merge-file.c
+++ b/merge-file.c
@@ -61,6 +61,7 @@ static int generate_common_file(mmfile_t *res,
mmfile_t *f1, mmfile_t *f2)
xdemitconf_t xecfg;
xdemitcb_t ecb;
+ memset(&xpp, 0, sizeof(xpp));
xpp.flags = XDF_NEED_MINIMAL;
memset(&xecfg, 0, sizeof(xecfg));
xecfg.ctxlen = 3;
--
1.6.0.3.514.g2f91b
^ permalink raw reply related
* [PATCH 1/5] blame: inline get_patch()
From: René Scharfe @ 2008-10-25 13:30 UTC (permalink / raw)
To: Brian Downing; +Cc: Junio C Hamano, git
In-Reply-To: <48BF0FBF.3010104@lsrfire.ath.cx>
Inline get_patch() to its only call site as a preparation for getting rid
of struct patch. Also we don't need to check the ptr members because
fill_origin_blob() already did, and the caller didn't check for NULL
anyway, so drop the test.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
builtin-blame.c | 31 +++++++++++--------------------
1 files changed, 11 insertions(+), 20 deletions(-)
diff --git a/builtin-blame.c b/builtin-blame.c
index 48cc0c1..593b539 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -542,25 +542,6 @@ static struct patch *compare_buffer(mmfile_t *file_p, mmfile_t *file_o,
return state.ret;
}
-/*
- * Run diff between two origins and grab the patch output, so that
- * we can pass blame for lines origin is currently suspected for
- * to its parent.
- */
-static struct patch *get_patch(struct origin *parent, struct origin *origin)
-{
- mmfile_t file_p, file_o;
- struct patch *patch;
-
- fill_origin_blob(parent, &file_p);
- fill_origin_blob(origin, &file_o);
- if (!file_p.ptr || !file_o.ptr)
- return NULL;
- patch = compare_buffer(&file_p, &file_o, 0);
- num_get_patch++;
- return patch;
-}
-
static void free_patch(struct patch *p)
{
free(p->chunks);
@@ -824,12 +805,22 @@ static int pass_blame_to_parent(struct scoreboard *sb,
{
int i, last_in_target, plno, tlno;
struct patch *patch;
+ mmfile_t file_p, file_o;
last_in_target = find_last_in_target(sb, target);
if (last_in_target < 0)
return 1; /* nothing remains for this target */
- patch = get_patch(parent, target);
+ /*
+ * Run diff between two origins and grab the patch output, so that
+ * we can pass blame for lines origin is currently suspected for
+ * to its parent.
+ */
+ fill_origin_blob(parent, &file_p);
+ fill_origin_blob(target, &file_o);
+ patch = compare_buffer(&file_p, &file_o, 0);
+ num_get_patch++;
+
plno = tlno = 0;
for (i = 0; i < patch->num; i++) {
struct chunk *chunk = &patch->chunks[i];
--
1.6.0.3.514.g2f91b
^ permalink raw reply related
* [PATCH 1/2] Fix git branch -m for symrefs.
From: Miklos Vajna @ 2008-10-25 12:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Brandon Casey, git
In-Reply-To: <cover.1224939436.git.vmiklos@frugalware.org>
This had two problems with symrefs. First, it copied the actual sha1
instead of the "pointer", second it failed to remove the old ref after a
successful rename.
Given that till now delete_ref() always dereferenced symrefs, a new
parameters has been introduced to delete_ref() to allow deleting refs
without a dereference.
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
builtin-branch.c | 2 +-
builtin-receive-pack.c | 2 +-
builtin-remote.c | 4 +-
builtin-reset.c | 2 +-
builtin-send-pack.c | 2 +-
builtin-tag.c | 2 +-
builtin-update-ref.c | 2 +-
cache.h | 2 +-
refs.c | 56 ++++++++++++++++++++++++++++++------------------
t/t3200-branch.sh | 9 +++++++
10 files changed, 53 insertions(+), 30 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 8d634ff..2b3613f 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -160,7 +160,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
continue;
}
- if (delete_ref(name, sha1)) {
+ if (delete_ref(name, sha1, 0)) {
error("Error deleting %sbranch '%s'", remote,
argv[i]);
ret = 1;
diff --git a/builtin-receive-pack.c b/builtin-receive-pack.c
index 45e3cd9..ab5fa1c 100644
--- a/builtin-receive-pack.c
+++ b/builtin-receive-pack.c
@@ -224,7 +224,7 @@ static const char *update(struct command *cmd)
warning ("Allowing deletion of corrupt ref.");
old_sha1 = NULL;
}
- if (delete_ref(name, old_sha1)) {
+ if (delete_ref(name, old_sha1, 0)) {
error("failed to delete %s", name);
return "failed to delete";
}
diff --git a/builtin-remote.c b/builtin-remote.c
index a5883df..3f2113c 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -441,7 +441,7 @@ static int remove_branches(struct string_list *branches)
const char *refname = item->string;
unsigned char *sha1 = item->util;
- if (delete_ref(refname, sha1))
+ if (delete_ref(refname, sha1, 0))
result |= error("Could not remove branch %s", refname);
}
return result;
@@ -669,7 +669,7 @@ static int prune(int argc, const char **argv)
const char *refname = states.stale.items[i].util;
if (!dry_run)
- result |= delete_ref(refname, NULL);
+ result |= delete_ref(refname, NULL, 0);
printf(" * [%s] %s\n", dry_run ? "would prune" : "pruned",
abbrev_ref(refname, "refs/remotes/"));
diff --git a/builtin-reset.c b/builtin-reset.c
index 16e6bb2..9514b77 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -279,7 +279,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
}
else if (old_orig)
- delete_ref("ORIG_HEAD", old_orig);
+ delete_ref("ORIG_HEAD", old_orig, 0);
prepend_reflog_action("updating HEAD", msg, sizeof(msg));
update_ref_status = update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 910db92..bbf6e0a 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -234,7 +234,7 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
if (args.verbose)
fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
if (ref->deletion) {
- delete_ref(rs.dst, NULL);
+ delete_ref(rs.dst, NULL, 0);
} else
update_ref("update by push", rs.dst,
ref->new_sha1, NULL, 0, 0);
diff --git a/builtin-tag.c b/builtin-tag.c
index b13fa34..1ff7b37 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -125,7 +125,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
static int delete_tag(const char *name, const char *ref,
const unsigned char *sha1)
{
- if (delete_ref(ref, sha1))
+ if (delete_ref(ref, sha1, 0))
return 1;
printf("Deleted tag '%s'\n", name);
return 0;
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index 56a0b1b..d8f3142 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -48,7 +48,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
die("%s: not a valid old SHA1", oldval);
if (delete)
- return delete_ref(refname, oldval ? oldsha1 : NULL);
+ return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
else
return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
diff --git a/cache.h b/cache.h
index b0edbf9..9951952 100644
--- a/cache.h
+++ b/cache.h
@@ -434,7 +434,7 @@ extern int commit_locked_index(struct lock_file *);
extern void set_alternate_index_output(const char *);
extern int close_lock_file(struct lock_file *);
extern void rollback_lock_file(struct lock_file *);
-extern int delete_ref(const char *, const unsigned char *sha1);
+extern int delete_ref(const char *, const unsigned char *sha1, int flags);
/* Environment bits from configuration mechanism */
extern int trust_executable_bit;
diff --git a/refs.c b/refs.c
index 0a126fa..93a61e1 100644
--- a/refs.c
+++ b/refs.c
@@ -921,25 +921,32 @@ static int repack_without_ref(const char *refname)
return commit_lock_file(&packlock);
}
-int delete_ref(const char *refname, const unsigned char *sha1)
+int delete_ref(const char *refname, const unsigned char *sha1, int flags)
{
struct ref_lock *lock;
- int err, i, ret = 0, flag = 0;
+ int err, i = 0, ret = 0, flag = 0;
+ char *path;
lock = lock_ref_sha1_basic(refname, sha1, 0, &flag);
if (!lock)
return 1;
if (!(flag & REF_ISPACKED)) {
/* loose */
- i = strlen(lock->lk->filename) - 5; /* .lock */
- lock->lk->filename[i] = 0;
- err = unlink(lock->lk->filename);
+ if (!(flags & REF_NODEREF)) {
+ i = strlen(lock->lk->filename) - 5; /* .lock */
+ lock->lk->filename[i] = 0;
+ path = lock->lk->filename;
+ } else {
+ path = git_path(refname);
+ }
+ err = unlink(path);
if (err && errno != ENOENT) {
ret = 1;
error("unlink(%s) failed: %s",
- lock->lk->filename, strerror(errno));
+ path, strerror(errno));
}
- lock->lk->filename[i] = '.';
+ if (!(flags & REF_NODEREF))
+ lock->lk->filename[i] = '.';
}
/* removing the loose one could have resurrected an earlier
* packed one. Also, if it was not loose we need to repack
@@ -964,10 +971,15 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
struct ref_lock *lock;
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+ const char *symref = NULL;
+ int is_symref = 0;
if (S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldref);
+ symref = resolve_ref(oldref, orig_sha1, 0, &flag);
+ if (flag & REF_ISSYMREF)
+ is_symref = 1;
if (!resolve_ref(oldref, orig_sha1, 1, &flag))
return error("refname %s not found", oldref);
@@ -988,12 +1000,12 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
return error("unable to move logfile logs/%s to tmp-renamed-log: %s",
oldref, strerror(errno));
- if (delete_ref(oldref, orig_sha1)) {
+ if (delete_ref(oldref, orig_sha1, REF_NODEREF)) {
error("unable to delete old %s", oldref);
goto rollback;
}
- if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1)) {
+ if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path("%s", newref))) {
error("Directory not empty: %s", newref);
@@ -1031,18 +1043,20 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
}
logmoved = log;
- lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
- if (!lock) {
- error("unable to lock %s for update", newref);
- goto rollback;
- }
-
- lock->force_write = 1;
- hashcpy(lock->old_sha1, orig_sha1);
- if (write_ref_sha1(lock, orig_sha1, logmsg)) {
- error("unable to write current sha1 into %s", newref);
- goto rollback;
- }
+ if (!is_symref) {
+ lock = lock_ref_sha1_basic(newref, NULL, 0, NULL);
+ if (!lock) {
+ error("unable to lock %s for update", newref);
+ goto rollback;
+ }
+ lock->force_write = 1;
+ hashcpy(lock->old_sha1, orig_sha1);
+ if (write_ref_sha1(lock, orig_sha1, logmsg)) {
+ error("unable to write current sha1 into %s", newref);
+ goto rollback;
+ }
+ } else
+ create_symref(newref, symref, logmsg);
return 0;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 2147eac..fdeb1f5 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -112,6 +112,15 @@ test_expect_success 'config information was renamed, too' \
"test $(git config branch.s.dummy) = Hello &&
test_must_fail git config branch.s/s/dummy"
+test_expect_success 'renaming a symref' \
+'
+ git symbolic-ref refs/heads/master2 refs/heads/master &&
+ git branch -m master2 master3 &&
+ git symbolic-ref refs/heads/master3 &&
+ test -f .git/refs/heads/master &&
+ ! test -f .git/refs/heads/master2
+'
+
test_expect_success \
'git branch -m u v should fail when the reflog for u is a symlink' '
git branch -l u &&
--
1.6.0.2
^ permalink raw reply related
* [PATCH 0/2] Fixes for git branch -m / update-ref --no-deref -d
From: Miklos Vajna @ 2008-10-25 12:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Brandon Casey, git
In-Reply-To: <7v63nh1sc7.fsf@gitster.siamese.dyndns.org>
On Fri, Oct 24, 2008 at 04:33:28PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> Hmm, remote_get() can read from all three supported places that you
> can
> define remotes. Could you explain what happens if the old remote is
> read
> from say $GIT_DIR/remotes/origin and you are renaming it to "upstream"
> with "git remote rename origin upstream"?
While trying to answer your question, I noticed that
rename_ref()/delete_ref() did not really handled symrefs.
Regarding rename_ref() (for users: git branch -m) I think you can't
create symrefs in the refs/heads namespace without using plumbing, so
most users are not affected.
Regarding delete_ref() (for users: git update-ref --no-deref -d) in most
repos you just have HEAD as symref and you never want to delete it, but
in case the user asks for it, I think we just have to do so.
Here are two patches to fix these issues (and in fact they will be
required for git remote rename as well).
Miklos Vajna (2):
Fix git branch -m for symrefs.
Fix git update-ref --no-deref -d.
builtin-branch.c | 2 +-
builtin-receive-pack.c | 2 +-
builtin-remote.c | 4 +-
builtin-reset.c | 2 +-
builtin-send-pack.c | 2 +-
builtin-tag.c | 2 +-
builtin-update-ref.c | 8 ++++--
cache.h | 2 +-
refs.c | 56 ++++++++++++++++++++++++++++++------------------
t/t1400-update-ref.sh | 7 ++++++
t/t3200-branch.sh | 9 +++++++
11 files changed, 64 insertions(+), 32 deletions(-)
^ permalink raw reply
* [PATCH 2/2] Fix git update-ref --no-deref -d.
From: Miklos Vajna @ 2008-10-25 12:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Brandon Casey, git
In-Reply-To: <cover.1224939436.git.vmiklos@frugalware.org>
Till now --no-deref was just ignored when deleting refs, fix this.
Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
builtin-update-ref.c | 8 +++++---
t/t1400-update-ref.sh | 7 +++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index d8f3142..378dc1b 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -13,7 +13,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
{
const char *refname, *oldval, *msg=NULL;
unsigned char sha1[20], oldsha1[20];
- int delete = 0, no_deref = 0;
+ int delete = 0, no_deref = 0, flags = 0;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, "reason", "reason of the update"),
OPT_BOOLEAN('d', NULL, &delete, "deletes the reference"),
@@ -47,9 +47,11 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
if (oldval && *oldval && get_sha1(oldval, oldsha1))
die("%s: not a valid old SHA1", oldval);
+ if (no_deref)
+ flags = REF_NODEREF;
if (delete)
- return delete_ref(refname, oldval ? oldsha1 : NULL, 0);
+ return delete_ref(refname, oldval ? oldsha1 : NULL, flags);
else
return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
- no_deref ? REF_NODEREF : 0, DIE_ON_ERR);
+ flags, DIE_ON_ERR);
}
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 04c2b16..8139cd6 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -75,6 +75,13 @@ test_expect_success "delete $m (by HEAD)" '
'
rm -f .git/$m
+cp -f .git/HEAD .git/HEAD.orig
+test_expect_success "delete symref without dereference" '
+ git update-ref --no-deref -d HEAD &&
+ ! test -f .git/HEAD
+'
+cp -f .git/HEAD.orig .git/HEAD
+
test_expect_success '(not) create HEAD with old sha1' "
test_must_fail git update-ref HEAD $A $B
"
--
1.6.0.2
^ permalink raw reply related
* [PATCH] gitk: Updated German translation.
From: Christian Stimming @ 2008-10-25 11:27 UTC (permalink / raw)
To: Paul Mackerras; +Cc: git
[-- Attachment #1: Type: text/plain, Size: 125 bytes --]
Patch against today's master of gitk-gui.git at git.kernel.org. Attached to
avoid whitespace problems.
Regards,
Christian
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gitk-Update-German-translation.patch --]
[-- Type: text/x-diff; charset="us-ascii"; name="0001-gitk-Update-German-translation.patch", Size: 3493 bytes --]
From 627fc16459e43f5577c16851332bf02a9d40595e Mon Sep 17 00:00:00 2001
From: Christian Stimming <stimming@tuhh.de>
Date: Sat, 25 Oct 2008 13:25:35 +0200
Subject: [PATCH] gitk: Update German translation.
This takes into account the most recent po file merge. For future reference:
The German translator strongly preferes not to have po file merged by the
maintainer (thus causing tons of conflicts for already existing local translations),
but instead to run "make update-po" on his own.
Signed-off-by: Christian Stimming <stimming@tuhh.de>
---
po/de.po | 51 +++++++++++++++++++++++++++++++++++++++++++--------
1 files changed, 43 insertions(+), 8 deletions(-)
diff --git a/po/de.po b/po/de.po
index c86cc2d..e0a6dee 100644
--- a/po/de.po
+++ b/po/de.po
@@ -7,8 +7,8 @@ msgid ""
msgstr ""
"Project-Id-Version: git-gui\n"
"Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2008-10-18 22:03+1100\n"
-"PO-Revision-Date: 2008-05-24 22:40+0200\n"
+"POT-Creation-Date: 2008-10-25 13:18+0200\n"
+"PO-Revision-Date: 2008-10-25 13:23+0200\n"
"Last-Translator: Christian Stimming <stimming@tuhh.de>\n"
"Language-Team: German\n"
"MIME-Version: 1.0\n"
@@ -19,6 +19,14 @@ msgstr ""
msgid "Couldn't get list of unmerged files:"
msgstr "Liste der nicht-zusammengeführten Dateien nicht gefunden:"
+#: gitk:272
+msgid "Error parsing revisions:"
+msgstr "Fehler beim Laden der Versionen:"
+
+#: gitk:327
+msgid "Error executing --argscmd command:"
+msgstr "Fehler beim --argscmd Kommando:"
+
#: gitk:340
msgid "No files selected: --merge specified but no files are unmerged."
msgstr ""
@@ -283,9 +291,9 @@ msgstr "Nur diesen hervorheben"
msgid "External diff"
msgstr "Externer Vergleich"
-#: gitk:2245
+#: gitk:2255
msgid "Blame parent commit"
-msgstr ""
+msgstr "Annotieren der Elternversion"
#: gitk:2488
msgid ""
@@ -471,7 +479,33 @@ msgstr "<%s-Minus>\tSchriftgröße verkleinern"
msgid "<F5>\t\tUpdate"
msgstr "<F5>\t\tAktualisieren"
-#: gitk:3200
+#: gitk:2979
+#, tcl-format
+msgid "Error getting \"%s\" from %s:"
+msgstr "Fehler beim Holen von »%s« von »%s«:"
+
+#: gitk:3036 gitk:3045
+#, tcl-format
+msgid "Error creating temporary directory %s:"
+msgstr "Fehler beim Erzeugen eines temporären Verzeichnisses »%s«:"
+
+#: gitk:3058
+msgid "command failed:"
+msgstr "Kommando fehlgeschlagen:"
+
+#: gitk:3078
+msgid "No such commit"
+msgstr "Version nicht gefunden"
+
+#: gitk:3083
+msgid "git gui blame: command failed:"
+msgstr "git gui blame: Kommando fehlgeschlagen:"
+
+#: gitk:3092
+msgid "External diff viewer failed:"
+msgstr "Externes Vergleich-(Diff-)Programm fehlgeschlagen:"
+
+#: gitk:3210
msgid "Gitk view definition"
msgstr "Gitk Ansichten"
@@ -692,9 +726,10 @@ msgstr "Bitte geben Sie einen Namen für den neuen Zweig an."
#, tcl-format
msgid "Commit %s is already included in branch %s -- really re-apply it?"
msgstr ""
-"Version »%s« ist bereits im Zweig »%s« enthalten -- trotzdem erneut eintragen?"
+"Version »%s« ist bereits im Zweig »%s« enthalten -- trotzdem erneut "
+"eintragen?"
-#: gitk:7708
+#: gitk:7718
msgid "Cherry-picking"
msgstr "Version pflücken"
@@ -836,7 +871,7 @@ msgstr "Vergleich nur für angezeigte Pfade"
#: gitk:9414
msgid "Support per-file encodings"
-msgstr ""
+msgstr "Zeichenkodierung pro Datei ermitteln"
#: gitk:9421
msgid "External diff tool"
--
1.6.0.rc1.34.g0fe8c
^ permalink raw reply related
* Re: [PATCH 3/7] gitk: Allow starting gui blame for a specific line.
From: Paul Mackerras @ 2008-10-25 11:57 UTC (permalink / raw)
To: Alexander Gavrilov; +Cc: git
In-Reply-To: <200810241213.01337.angavrilov@gmail.com>
Alexander Gavrilov writes:
> Good point. How about this variant? I renamed the menu item, and
> changed the code to blame the current commit if:
Sounds good, just a couple of minor nits...
> + # Now scan the lines to determine offset within the hunk
> + set parent {}
> + set dline 0
> + set s_lno [lindex [split $s_lix "."] 0]
> +
> + for {set i $line} {$i > $s_lno} {incr i -1} {
> + set c_line [$ctext get $i.0 "$i.0 + 1 lines"]
> + # Determine if the line is removed
> + set chunk [string range $c_line 0 [llength $base_lines]-2]
You need an [expr]:
set chunk [string range $c_line 0 [expr {[llength $base_lines] - 2}]]
> + set removed_idx [string first "-" $chunk]
> + # Choose a parent index
> + if {$parent eq {}} {
> + if {$removed_idx >= 0} {
> + set parent $removed_idx
> + incr parent
> + } else {
> + set unchanged_idx [string first " " $chunk]
> + if {$unchanged_idx >= 0} {
> + set parent $unchanged_idx
> + incr parent
> + } else {
> + # blame the current commit
> + set parent 0
> + }
> + }
> + }
I like this better than the previous version, but it would turn out a
bit simpler if you use parent = -1 to indicate that we're blaming the
current commit, and then increment it right at the end.
> + # then count other lines that belong to it
> + if {$parent > 0} {
> + set code [string index $c_line $parent-1]
Once again you need an [expr].
Apart from those things, it looks good. If you like I will fix those
things and commit it.
Paul.
^ permalink raw reply
* Re: [RFC] Zit: the git-based single file content tracker
From: Giuseppe Bilotta @ 2008-10-25 10:30 UTC (permalink / raw)
To: Jakub Narebski; +Cc: David Lang, Junio C Hamano, Jean-Luc Herren, git
In-Reply-To: <200810251110.25704.jnareb@gmail.com>
On Sat, Oct 25, 2008 at 11:10 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Sat, 25 Oct 2008, Giuseppe Bilotta wrote:
>
>> The user .zitignore file is probably the best approach: we can create
>> it ourselves (usually), and even if Git doesn't expand the pathname
>> itself, we can just use an absolute path. I'll go that way.
>
> First, absolute path to ~/.zitignore is a bit fragile: what if layout
> of home directories for users change, for example because of increasing
> number of users some fan-out is required (/home/nick -> /home/2/nick)?
> Second, ~/.zitignore looks like something that user can change; if
> you install zit, it can install libexec/zitignore somewhere... or just
> use ./zit/excludes (with 'do not edit' comment perhaps...).
(Actually, I just found another interesting thing about the config, in
that it stores the path to the work tree. This is not a problem,
though, because zit_setup() sets GIT_WORK_TREE.)
As I said, I don't like depending on stuff that needs to be installed.
For example, what about user (non-system) installs? the libexec (or
whatever) solution would have the same problem as the ~/.zitignore
solution, with the moving $HOME.
I guess this leaves the .zit/ solution as the most robust one,
although it's not the most space-effective, especially if you have
many directories, each with a single tracked file. On the plus side,
going for the .zit/ solution and dropping support for .somefile.git/
means some significant code semplification.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [PATCH 2/3] Add -n/--no-prompt option to mergetool
From: William Pursell @ 2008-10-25 10:11 UTC (permalink / raw)
To: Charles Bailey
Cc: Junio C Hamano, git, Jeff King, Andreas Ericsson,
Theodore Ts'o
In-Reply-To: <20081024225539.GA6119@hashpling.org>
Charles Bailey wrote:
> On Fri, Oct 24, 2008 at 11:32:17PM +0100, William Pursell wrote:
>> If the short option is dropped, the config option should
>> probably associated with mergetool.<tool>.interactive rather
>> than mergetool.interactive. (s/interactive/whatever)
>
> I'm not sure I understand your reasoning. The no-prompt/interactive
> option affects the behaviour of the mergetool script independent of
> which particular merge tool is being used. Why should the presence or
> absence of a short option affect whether the config option is global
> or per tool?
>
My thinking is that when using an interactive tool
like vimdiff, the user is probably not going to
care as much about being prompted, or may prefer
to have the prompt in that situation. However,
if they've written a script to do the merge
non-interactively, then the prompt is undesirable.
So a person might want to be prompted with
git mergetool -t vimdiff, and prefer no prompt
with git mergetool -t my-script. Being able
to configure the behavior on a per-tool
basis would allow that.
--
William Pursell
^ permalink raw reply
* Problem with git filter-branch
From: Pascal Obry @ 2008-10-25 9:33 UTC (permalink / raw)
To: git list
I'm using Git svn but I do not think this matters.
I'm used to add manually some branches from the Subversion repository. I
usually fetch the branch without following the parent then connect the
branch to the proper branch point (I really need to do this as the
Subversion repository structure is not following the standard layout).
Anyway, I used to run the following command:
$ git filter-branch --tag-name-filter cat --parent-filter "sed -e
's/^$/-p c96d4da294667de1800687d25340551683153002/'" svn-release_2_6
without problem, I now get this:
Namespace refs/original/ not empty
rm: cannot remove directory
`/home/obry/dev/repositories/git/proj/.git-rewrite': Directory not empty
The .git-rewrite is empty and when the command return I can do:
$ rmdir .git-rewrite
without problem.
Is this is known issue? Any idea?
Thanks,
Pascal.
--
--|------------------------------------------------------
--| Pascal Obry Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--| http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595
^ permalink raw reply
* Re: [RFC] Zit: the git-based single file content tracker
From: Jakub Narebski @ 2008-10-25 9:10 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: David Lang, Junio C Hamano, Jean-Luc Herren, git
In-Reply-To: <cb7bb73a0810250048q7ad8595bt565de05ec2ec37cb@mail.gmail.com>
On Sat, 25 Oct 2008, Giuseppe Bilotta wrote:
> On Fri, Oct 24, 2008 at 10:30 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> > Well, with all zit repositories in '.zit/' directory (similar to RCS/)
> > you could have point core.excludesfile to _common_ '.zit/excludes';
> > the pattern doesn't change from zit repository to zit repository?
> >
> > You could even use per-user ~/.zitignore (I'm not sure if git expands
> > '~' in paths; there was some patch for it, but was it accepted?) [...]
[...]
> The user .zitignore file is probably the best approach: we can create
> it ourselves (usually), and even if Git doesn't expand the pathname
> itself, we can just use an absolute path. I'll go that way.
First, absolute path to ~/.zitignore is a bit fragile: what if layout
of home directories for users change, for example because of increasing
number of users some fan-out is required (/home/nick -> /home/2/nick)?
Second, ~/.zitignore looks like something that user can change; if
you install zit, it can install libexec/zitignore somewhere... or just
use ./zit/excludes (with 'do not edit' comment perhaps...).
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: [RFC] Zit: the git-based single file content tracker
From: Giuseppe Bilotta @ 2008-10-25 7:48 UTC (permalink / raw)
To: Jakub Narebski; +Cc: David Lang, Junio C Hamano, Jean-Luc Herren, git
In-Reply-To: <200810242230.49238.jnareb@gmail.com>
On Fri, Oct 24, 2008 at 10:30 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Well, with all zit repositories in '.zit/' directory (similar to RCS/)
> you could have point core.excludesfile to _common_ '.zit/excludes';
> the pattern doesn't change from zit repository to zit repository?
>
> You could even use per-user ~/.zitignore (I'm not sure if git expands
> '~' in paths; there was some patch for it, but was it accepted?) or
> system-wide /usr/lib/zitignore or /usr/libexec/zitignore file.
System-wide means maximum space save, but it require system
administration to install Zit, and considering that one of the things
I love of Zit now is its being self contained, I would rather not
depend on anything system-wide anyway.
The user .zitignore file is probably the best approach: we can create
it ourselves (usually), and even if Git doesn't expand the pathname
itself, we can just use an absolute path. I'll go that way.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [PATCH 4/7] textconv: don't convert for every operation
From: Jeff King @ 2008-10-25 7:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <7vhc71b5ai.fsf@gitster.siamese.dyndns.org>
On Fri, Oct 24, 2008 at 10:41:09PM -0700, Junio C Hamano wrote:
> Isn't this function "fill_mmfile()" and its callers leaky as a sieve?
Yes, it is (for the textconv case; the other is unaffected). That was
another reason I had attached the data to the diff_filespec, but
obviously I forgot about that when re-rolling the patch.
> I am also somewhat worried about the performance impact of running
> get_textconv() to the same filespec many times when no textconv is
> defined, which is the normal case we should optimize for. It appears that
> diff_filespec_load_driver() is optimized for a wrong case (i.e. "we
> already know this needs a custom driver and we know which one").
No, it is the same as before. We always end up with a driver at the end
of the function, so further calls will be no-ops. So we do exactly one
attribute lookup per filespec, caching the result.
> I am inclined to suggest reverting the whole series (including the ones
> that are already in 'master') and start over from scratch, limiting the
> run_textconv() to only inside diff.c::builtin_diff() (in the else {} block
> where "Crazy xcl interfaces.." comment appears).
I am not terribly opposed to that (I was quite surprised to see the
original make it to 'next', let alone 'master'). OTOH, the real reason
to do so would be to keep a clean history, and it is already too late
for that.
I think it makes sense to figure out _what_ needs fixed first, because
it might be somewhat minor. So far I see:
- leak from fill_mmfile; this definitely needs fixed. The quick fix is
minor (free if we did a textconv). A more involved fix is to pull it
out of fill_mmfile entirely and put the code directly into
builtin_diff, which would be part of a re-roll of this latest
series. But see below.
- Keep fill_mmfile allocation semantics clear. I was trying to keep
it simple for other fill_mmfile callers to opt-in to textconv, even
if they chose to do it by some user-controlled mechanism instead of
by default (e.g., diff.TextconvDiffstat or something). But maybe
that is not of value to us. Again, that is a re-roll of this series.
- performance considerations with driver loading. I believe this is a
non-issue. So either you are reading the code wrong, or I am not
understanding your concern correctly (or _I_ am reading the code
wrong, of course).
Others?
-Peff
^ permalink raw reply
* Re* [PATCH/RFC] Introduce a built-in attribute "text"
From: Junio C Hamano @ 2008-10-25 6:51 UTC (permalink / raw)
To: Michael J Gruber; +Cc: git
In-Reply-To: <1224856547-30533-1-git-send-email-git@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> "text is the opposite of "binary": It sets the attributes "crlf" and
> "diff". It is needed because attribute macros can't be negated,
If the reason for this patch is really (and only) because macros cannot be
negated, perhaps we should be allowing to negate them? That is, given:
[attr] binary = -diff -crlf
*.bar !binary
you would set diff and crlf to TRUE for b.bar, because the normal action
is to set them to FALSE.
If the original attribute macro expands to anything but setting other
attributes to TRUE or FALSE (i.e. to a string value, or reseting to
UNSPECIFIED), or if a related attribute is not the exact opposite, you
cannot simply negate, but need to introduce a new attribute like your
patch does. For example, it is conceivable you might instead want to
define:
[attr] text = diff
i.e. without "crlf" bit.
Since what you want to do here is merely to negate what is already there,
it might make more sense to simply bite the bullet and give the attribute
mechanism the ability to negate an attribute macro.
This is of course untested (you can tell that by noticing that it does not
come with patch to the test scripts) nor undocumented, but it looks
obvious enough ;-). Originally we silently ignored attempts to negate or
to reset an attribute macro. We now allow you to negate an attribute
macro, and the definition of "negating" is:
- negation of setting to TRUE is setting to FALSE.
- negation of setting to FALSE is setting to TRUE.
- negation of anything else is itself.
The third one is arbitrary and people may want to come up with a different
behaviour (with solid argument explaining why that behaviour is more
sensible).
attr.c | 34 +++++++++++++++++++++++++++++-----
1 files changed, 29 insertions(+), 5 deletions(-)
diff --git c/attr.c w/attr.c
index 17f6a4d..0021cec 100644
--- c/attr.c
+++ w/attr.c
@@ -558,7 +558,24 @@ static int path_matches(const char *pathname, int pathlen,
return fnmatch(pattern, pathname + baselen, FNM_PATHNAME) == 0;
}
-static int fill_one(const char *what, struct match_attr *a, int rem)
+static const char *attr_negate(const char *value)
+{
+ if (value == ATTR__TRUE)
+ return ATTR__FALSE;
+ else if (value == ATTR__FALSE)
+ return ATTR__TRUE;
+ else
+ /*
+ * NEEDSWORK: here we define negation of setting
+ * to anything but TRUE and FALSE is itself. We
+ * may want to instead say it is not touching the
+ * attribute, in which case you could simply return
+ * NULL from here.
+ */
+ return value;
+}
+
+static int fill_one(const char *what, struct match_attr *a, int rem, int negated)
{
struct git_attr_check *check = check_all_attr;
int i;
@@ -569,6 +586,11 @@ static int fill_one(const char *what, struct match_attr *a, int rem)
const char *v = a->state[i].setto;
if (*n == ATTR__UNKNOWN) {
+ if (negated) {
+ v = attr_negate(v);
+ if (!v)
+ continue;
+ }
debug_set(what, a->u.pattern, attr, v);
*n = v;
rem--;
@@ -588,7 +610,7 @@ static int fill(const char *path, int pathlen, struct attr_stack *stk, int rem)
continue;
if (path_matches(path, pathlen,
a->u.pattern, base, strlen(base)))
- rem = fill_one("fill", a, rem);
+ rem = fill_one("fill", a, rem, 0);
}
return rem;
}
@@ -602,9 +624,11 @@ static int macroexpand(struct attr_stack *stk, int rem)
struct match_attr *a = stk->attrs[i];
if (!a->is_macro)
continue;
- if (check[a->u.attr->attr_nr].value != ATTR__TRUE)
- continue;
- rem = fill_one("expand", a, rem);
+ if (check[a->u.attr->attr_nr].value == ATTR__TRUE)
+ rem = fill_one("expand", a, rem, 0);
+ else if (check[a->u.attr->attr_nr].value == ATTR__FALSE)
+ rem = fill_one("expand", a, rem, 1);
+
}
return rem;
}
^ permalink raw reply related
* Re: [PATCH 4/7] textconv: don't convert for every operation
From: Junio C Hamano @ 2008-10-25 5:41 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025005256.GD23903@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> -static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
> +static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one,
> + int try_textconv)
> {
> const char *textconv;
>
> @@ -304,7 +305,7 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
> else if (diff_populate_filespec(one, 0))
> return -1;
>
> - if ((textconv = get_textconv(one))) {
> + if (try_textconv && (textconv = get_textconv(one))) {
> size_t size;
> mf->ptr = run_textconv(textconv, one, &size);
> if (!mf->ptr)
> @@ -1396,7 +1397,7 @@ static void builtin_diff(const char *name_a,
> }
> }
Isn't this function "fill_mmfile()" and its callers leaky as a sieve?
The original fill_mmfile() was to only borrow one->data and users of
mmfile_t never had to worry about freeing what's in mf->ptr (and freeing
them would have been actively wrong).
I am also somewhat worried about the performance impact of running
get_textconv() to the same filespec many times when no textconv is
defined, which is the normal case we should optimize for. It appears that
diff_filespec_load_driver() is optimized for a wrong case (i.e. "we
already know this needs a custom driver and we know which one").
I am inclined to suggest reverting the whole series (including the ones
that are already in 'master') and start over from scratch, limiting the
run_textconv() to only inside diff.c::builtin_diff() (in the else {} block
where "Crazy xcl interfaces.." comment appears).
^ permalink raw reply
* Re: Performance impact of a large number of commits
From: david @ 2008-10-25 5:29 UTC (permalink / raw)
To: Samuel Abels; +Cc: git
In-Reply-To: <1224911915.7566.35.camel@localhost>
On Sat, 25 Oct 2008, Samuel Abels wrote:
> On Fri, 2008-10-24 at 13:11 -0700, david@lang.hm wrote:
>>> git commit explicitly (i.e., walking the tree to stat files for finding
>>> changes is not necessary).
>>
>> I suspect that your limits would be filesystem/OS limits more than git
>> limits
>>
>> at 5-10 files/commit you are going to be creating .5-1m files/day, even
>> spread across 256 directories this is going to be a _lot_ of files.
>
> The files are organized in a way that places no more than ~1.000 files
> into each directory. Will Git create a directory containing a larger
> number of object files? I can see that this would be a problem in our
> use case.
when git stores the copies of the files it does a sha1 hash of the file
contents and then stores the file in the directory
.git/objects/<first two digits of the hash>/<hash>
this means that if you have files that have the same content they all fold
togeather, but with lots of files changing rapidly the result is a lot of
files in these object directories.
it would be a pretty minor change to git to have it use more directories
(in fact, there's another thread going on today where people are looking
at making this configurable, in that case to reduce the number of
directories)
the other storage format that git has is the pack file. it takes a bunch
of the objects, does some comparisons between them (to find duplicate bits
of files), and then stores the result (base files plus deltas to re-create
other files). the resulting compression is _extremely_ efficiant, and it
collapses many file objects into one pack file (addressing the issues of
many files in one directory)
>> packing this may help (depending on how much the files change), but with
>> this many files the work of doing the packing would be expensive.
>
> We can probably do that even if it takes several hours.
my concern is that spending time creating the pack files will mean that
you don't have time to insert the new files.
that being said, there may be other ways of dealing with this data rather
than putting it into files and then adding it to the git repository.
Git has a fast-import streaming format that is designed for programs to
use that are converting repositories from other SCM systems. if you can
tell more about what you are doing (how the data is being gathered, are
the files re-created for each commit, or are they being modified? if they
are being modified is it appending data, changing some data, or randomly
writing throughout the file? etc) there may be some other options
available.
at this point I don't know if git can work for you or not, but I'm pretty
sure nothing else will have a chance with your size.
David Lang
^ permalink raw reply
* Re: Performance impact of a large number of commits
From: Samuel Abels @ 2008-10-25 5:18 UTC (permalink / raw)
To: david; +Cc: git
In-Reply-To: <alpine.DEB.1.10.0810241301430.27333@asgard.lang.hm>
On Fri, 2008-10-24 at 13:11 -0700, david@lang.hm wrote:
> > git commit explicitly (i.e., walking the tree to stat files for finding
> > changes is not necessary).
>
> I suspect that your limits would be filesystem/OS limits more than git
> limits
>
> at 5-10 files/commit you are going to be creating .5-1m files/day, even
> spread across 256 directories this is going to be a _lot_ of files.
The files are organized in a way that places no more than ~1.000 files
into each directory. Will Git create a directory containing a larger
number of object files? I can see that this would be a problem in our
use case.
> packing this may help (depending on how much the files change), but with
> this many files the work of doing the packing would be expensive.
We can probably do that even if it takes several hours.
-Samuel
^ permalink raw reply
* [PATCH 7/7] only textconv regular files
From: Jeff King @ 2008-10-25 0:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
We treat symlinks as text containing the results of the
symlink, so it doesn't make much sense to text-convert them.
Similarly gitlink components just end up as the text
"Subproject commit $sha1", which we should leave intact.
Note that a typechange may be broken into two parts: the
removal of the old part and the addition of the new. In that
case, we _do_ show the textconv for any part which is the
addition or removal of a file we would ordinarily textconv,
since it is purely acting on the file contents.
Signed-off-by: Jeff King <peff@peff.net>
---
Similar to before, except we already have the expect_failure test now.
diff.c | 2 ++
t/t4030-diff-textconv.sh | 2 +-
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/diff.c b/diff.c
index e3bb1ed..a87ce97 100644
--- a/diff.c
+++ b/diff.c
@@ -282,6 +282,8 @@ static const char *get_textconv(struct diff_filespec *one)
{
if (!DIFF_FILE_VALID(one))
return NULL;
+ if (!S_ISREG(one->mode))
+ return NULL;
diff_filespec_load_driver(one);
return one->driver->textconv;
}
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 1df48ae..3945731 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -104,7 +104,7 @@ index ad8b3d2..67be421
\ No newline at end of file
EOF
# make a symlink the hard way that works on symlink-challenged file systems
-test_expect_failure 'textconv does not act on symlinks' '
+test_expect_success 'textconv does not act on symlinks' '
echo -n frotz > file &&
git add file &&
git ls-files -s | sed -e s/100644/120000/ |
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 6/7] document the diff driver textconv feature
From: Jeff King @ 2008-10-25 0:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
This patch also changes the term "custom diff driver" to
"external diff driver"; now that there are more facets of a
"custom driver" than just external diffing, it makes sense
to refer to the configuration of "diff.foo.*" as the "foo
diff driver", with "diff.foo.command" as the "external
driver for foo".
Signed-off-by: Jeff King <peff@peff.net>
---
Same as before.
Documentation/gitattributes.txt | 66 +++++++++++++++++++++++++++++++--------
1 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 2694559..314e2d3 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -213,10 +213,12 @@ with `crlf`, and then `ident` and fed to `filter`.
Generating diff text
~~~~~~~~~~~~~~~~~~~~
-The attribute `diff` affects if 'git-diff' generates textual
-patch for the path or just says `Binary files differ`. It also
-can affect what line is shown on the hunk header `@@ -k,l +n,m @@`
-line.
+The attribute `diff` affects how 'git' generates diffs for particular
+files. It can tell git whether to generate a textual patch for the path
+or to treat the path as a binary file. It can also affect what line is
+shown on the hunk header `@@ -k,l +n,m @@` line, tell git to use an
+external command to generate the diff, or ask git to convert binary
+files to a text format before generating the diff.
Set::
@@ -227,7 +229,8 @@ Set::
Unset::
A path to which the `diff` attribute is unset will
- generate `Binary files differ`.
+ generate `Binary files differ` (or a binary patch, if
+ binary patches are enabled).
Unspecified::
@@ -238,21 +241,21 @@ Unspecified::
String::
- Diff is shown using the specified custom diff driver.
- The driver program is given its input using the same
- calling convention as used for GIT_EXTERNAL_DIFF
- program. This name is also used for custom hunk header
- selection.
+ Diff is shown using the specified diff driver. Each driver may
+ specify one or more options, as described in the following
+ section. The options for the diff driver "foo" are defined
+ by the configuration variables in the "diff.foo" section of the
+ git config file.
-Defining a custom diff driver
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+Defining an external diff driver
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
The definition of a diff driver is done in `gitconfig`, not
`gitattributes` file, so strictly speaking this manual page is a
wrong place to talk about it. However...
-To define a custom diff driver `jcdiff`, add a section to your
+To define an external diff driver `jcdiff`, add a section to your
`$GIT_DIR/config` file (or `$HOME/.gitconfig` file) like this:
----------------------------------------------------------------
@@ -328,6 +331,43 @@ patterns are available:
- `tex` suitable for source code for LaTeX documents.
+Performing text diffs of binary files
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Sometimes it is desirable to see the diff of a text-converted
+version of some binary files. For example, a word processor
+document can be converted to an ASCII text representation, and
+the diff of the text shown. Even though this conversion loses
+some information, the resulting diff is useful for human
+viewing (but cannot be applied directly).
+
+The `textconv` config option is used to define a program for
+performing such a conversion. The program should take a single
+argument, the name of a file to convert, and produce the
+resulting text on stdout.
+
+For example, to show the diff of the exif information of a
+file instead of the binary information (assuming you have the
+exif tool installed):
+
+------------------------
+[diff "jpg"]
+ textconv = exif
+------------------------
+
+NOTE: The text conversion is generally a one-way conversion;
+in this example, we lose the actual image contents and focus
+just on the text data. This means that diffs generated by
+textconv are _not_ suitable for applying. For this reason,
+only `git diff` and the `git log` family of commands (i.e.,
+log, whatchanged, show) will perform text conversion. `git
+format-patch` will never generate this output. If you want to
+send somebody a text-converted diff of a binary file (e.g.,
+because it quickly conveys the changes you have made), you
+should generate it separately and send it as a comment _in
+addition to_ the usual binary diff that you might send.
+
+
Performing a three-way merge
~~~~~~~~~~~~~~~~~~~~~~~~~~~~
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 5/7] userdiff: require explicitly allowing textconv
From: Jeff King @ 2008-10-25 0:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
Diffs that have been produced with textconv almost certainly
cannot be applied, so we want to be careful not to generate
them in things like format-patch.
This introduces a new diff options, ALLOW_TEXTCONV, which
controls this behavior. It is off by default, but is
explicitly turned on for the "log" family of commands, as
well as the "diff" porcelain.
Because both text conversion and external diffing are
controlled by these diff options, we can get rid of the
"plumbing versus porcelain" distinction when reading the
config. This was an attempt to control the same thing, but
suffered from being too coarse-grained.
Signed-off-by: Jeff King <peff@peff.net>
---
Same idea as before, but adapted to the new calling convention for
fill_mmfile. It makes the "did we textconv this?" test a little bit
more hairy. We could perhaps just set a "we did a textconv" flag on the
mmfile, but I am also fine with it as-is (especially since there is only
one call-site).
builtin-diff.c | 1 +
builtin-log.c | 1 +
diff.c | 27 ++++++++++++---------------
diff.h | 1 +
t/t4030-diff-textconv.sh | 2 +-
userdiff.c | 10 +---------
userdiff.h | 3 +--
7 files changed, 18 insertions(+), 27 deletions(-)
diff --git a/builtin-diff.c b/builtin-diff.c
index 9c8c295..2de5834 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -300,6 +300,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
}
DIFF_OPT_SET(&rev.diffopt, ALLOW_EXTERNAL);
DIFF_OPT_SET(&rev.diffopt, RECURSIVE);
+ DIFF_OPT_SET(&rev.diffopt, ALLOW_TEXTCONV);
/*
* If the user asked for our exit code then don't start a
diff --git a/builtin-log.c b/builtin-log.c
index a0944f7..75d698f 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -59,6 +59,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
} else
die("unrecognized argument: %s", arg);
}
+ DIFF_OPT_SET(&rev->diffopt, ALLOW_TEXTCONV);
}
/*
diff --git a/diff.c b/diff.c
index fcdfd7b..e3bb1ed 100644
--- a/diff.c
+++ b/diff.c
@@ -93,12 +93,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
if (!strcmp(var, "diff.external"))
return git_config_string(&external_diff_cmd_cfg, var, value);
- switch (userdiff_config_porcelain(var, value)) {
- case 0: break;
- case -1: return -1;
- default: return 0;
- }
-
return git_diff_basic_config(var, value, cb);
}
@@ -109,6 +103,12 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return 0;
}
+ switch (userdiff_config(var, value)) {
+ case 0: break;
+ case -1: return -1;
+ default: return 0;
+ }
+
if (!prefixcmp(var, "diff.color.") || !prefixcmp(var, "color.diff.")) {
int slot = parse_diff_color_slot(var, 11);
if (!value)
@@ -123,12 +123,6 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
return 0;
}
- switch (userdiff_config_basic(var, value)) {
- case 0: break;
- case -1: return -1;
- default: return 0;
- }
-
return git_color_default_config(var, value, cb);
}
@@ -1397,12 +1391,15 @@ static void builtin_diff(const char *name_a,
}
}
- if (fill_mmfile(&mf1, one, 1) < 0 || fill_mmfile(&mf2, two, 1) < 0)
+ if (fill_mmfile(&mf1, one, DIFF_OPT_TST(o, ALLOW_TEXTCONV)) < 0 ||
+ fill_mmfile(&mf2, two, DIFF_OPT_TST(o, ALLOW_TEXTCONV)) < 0)
die("unable to read files to diff");
if (!DIFF_OPT_TST(o, TEXT) &&
- ( (diff_filespec_is_binary(one) && !get_textconv(one)) ||
- (diff_filespec_is_binary(two) && !get_textconv(two)) )) {
+ ( (diff_filespec_is_binary(one) &&
+ !(DIFF_OPT_TST(o, ALLOW_TEXTCONV) && get_textconv(one))) ||
+ (diff_filespec_is_binary(two) &&
+ !(DIFF_OPT_TST(o, ALLOW_TEXTCONV) && get_textconv(two))) )) {
/* Quite common confusing case */
if (mf1.size == mf2.size &&
!memcmp(mf1.ptr, mf2.ptr, mf1.size))
diff --git a/diff.h b/diff.h
index a49d865..42582ed 100644
--- a/diff.h
+++ b/diff.h
@@ -65,6 +65,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
#define DIFF_OPT_IGNORE_SUBMODULES (1 << 18)
#define DIFF_OPT_DIRSTAT_CUMULATIVE (1 << 19)
#define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20)
+#define DIFF_OPT_ALLOW_TEXTCONV (1 << 21)
#define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)
#define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)
#define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 090a21d..1df48ae 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -70,7 +70,7 @@ test_expect_success 'log produces text' '
test_cmp expect.text actual
'
-test_expect_failure 'format-patch produces binary' '
+test_expect_success 'format-patch produces binary' '
git format-patch --no-binary --stdout HEAD^ >patch &&
find_diff <patch >actual &&
test_cmp expect.binary actual
diff --git a/userdiff.c b/userdiff.c
index d95257a..3681062 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -120,7 +120,7 @@ static int parse_tristate(int *b, const char *k, const char *v)
return 1;
}
-int userdiff_config_basic(const char *k, const char *v)
+int userdiff_config(const char *k, const char *v)
{
struct userdiff_driver *drv;
@@ -130,14 +130,6 @@ int userdiff_config_basic(const char *k, const char *v)
return parse_funcname(&drv->funcname, k, v, REG_EXTENDED);
if ((drv = parse_driver(k, v, "binary")))
return parse_tristate(&drv->binary, k, v);
-
- return 0;
-}
-
-int userdiff_config_porcelain(const char *k, const char *v)
-{
- struct userdiff_driver *drv;
-
if ((drv = parse_driver(k, v, "command")))
return parse_string(&drv->external, k, v);
if ((drv = parse_driver(k, v, "textconv")))
diff --git a/userdiff.h b/userdiff.h
index f29c18f..ba29457 100644
--- a/userdiff.h
+++ b/userdiff.h
@@ -14,8 +14,7 @@ struct userdiff_driver {
const char *textconv;
};
-int userdiff_config_basic(const char *k, const char *v);
-int userdiff_config_porcelain(const char *k, const char *v);
+int userdiff_config(const char *k, const char *v);
struct userdiff_driver *userdiff_find_by_name(const char *name);
struct userdiff_driver *userdiff_find_by_path(const char *path);
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 4/7] textconv: don't convert for every operation
From: Jeff King @ 2008-10-25 0:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
Since we do the text conversion in fill_mmfile, the
conversion was happening any time we fed data to xdiff,
including diffstat, whitespace checking, etc.
This patch makes it optional for each caller, and
conservatively chooses to turn it on only for actual patch
generation.
Signed-off-by: Jeff King <peff@peff.net>
---
And this was part 2 of the big refactoring patch.
diff.c | 19 ++++++++++---------
t/t4030-diff-textconv.sh | 2 +-
2 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/diff.c b/diff.c
index 8fad215..fcdfd7b 100644
--- a/diff.c
+++ b/diff.c
@@ -292,7 +292,8 @@ static const char *get_textconv(struct diff_filespec *one)
return one->driver->textconv;
}
-static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
+static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one,
+ int try_textconv)
{
const char *textconv;
@@ -304,7 +305,7 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
else if (diff_populate_filespec(one, 0))
return -1;
- if ((textconv = get_textconv(one))) {
+ if (try_textconv && (textconv = get_textconv(one))) {
size_t size;
mf->ptr = run_textconv(textconv, one, &size);
if (!mf->ptr)
@@ -1396,7 +1397,7 @@ static void builtin_diff(const char *name_a,
}
}
- if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
+ if (fill_mmfile(&mf1, one, 1) < 0 || fill_mmfile(&mf2, two, 1) < 0)
die("unable to read files to diff");
if (!DIFF_OPT_TST(o, TEXT) &&
@@ -1486,7 +1487,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
data->added = count_lines(two->data, two->size);
goto free_and_return;
}
- if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
+ if (fill_mmfile(&mf1, one, 0) < 0 || fill_mmfile(&mf2, two, 0) < 0)
die("unable to read files to diff");
if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {
@@ -1528,7 +1529,7 @@ static void builtin_checkdiff(const char *name_a, const char *name_b,
data.o = o;
data.ws_rule = whitespace_rule(attr_path);
- if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
+ if (fill_mmfile(&mf1, one, 0) < 0 || fill_mmfile(&mf2, two, 0) < 0)
die("unable to read files to diff");
/*
@@ -2085,8 +2086,8 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
if (DIFF_OPT_TST(o, BINARY)) {
mmfile_t mf;
- if ((!fill_mmfile(&mf, one) && diff_filespec_is_binary(one)) ||
- (!fill_mmfile(&mf, two) && diff_filespec_is_binary(two)))
+ if ((!fill_mmfile(&mf, one, 0) && diff_filespec_is_binary(one)) ||
+ (!fill_mmfile(&mf, two, 0) && diff_filespec_is_binary(two)))
abbrev = 40;
}
strbuf_addf(&msg, "index %.*s..%.*s",
@@ -2983,8 +2984,8 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
diff_fill_sha1_info(p->one);
diff_fill_sha1_info(p->two);
- if (fill_mmfile(&mf1, p->one) < 0 ||
- fill_mmfile(&mf2, p->two) < 0)
+ if (fill_mmfile(&mf1, p->one, 0) < 0 ||
+ fill_mmfile(&mf2, p->two, 0) < 0)
return error("unable to read files to diff");
len1 = remove_space(p->one->path, strlen(p->one->path));
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index af94e1a..090a21d 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -80,7 +80,7 @@ cat >expect.stat <<'EOF'
file | Bin 2 -> 4 bytes
1 files changed, 0 insertions(+), 0 deletions(-)
EOF
-test_expect_failure 'diffstat does not run textconv' '
+test_expect_success 'diffstat does not run textconv' '
echo file diff=fail >.gitattributes &&
git diff --stat HEAD^ HEAD >actual &&
test_cmp expect.stat actual
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 3/7] textconv: assume text-converted contents are not binary
From: Jeff King @ 2008-10-25 0:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
Previously, we would use a 'textconv' filter when filling
the mmfile_t, but then still check the diff_filespec struct,
which was not modified at all by the text conversion,
whether it was binary. And in general, it was, since the
point of text conversion is to munge binary contents. This
meant that the user had to manually set "diff.foo.binary =
false" to convince the text conversion to actually happen.
This patch pulls the logic for "can we textconv this
diff_filespec?" into a separate function. We can then check
this to see if a filespec which is binary would have been
converted.
Signed-off-by: Jeff King <peff@peff.net>
---
This used to be part 1 of the big refactoring patch.
diff.c | 18 ++++++++++++++----
t/t4030-diff-textconv.sh | 4 ++--
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index d1fd594..8fad215 100644
--- a/diff.c
+++ b/diff.c
@@ -284,8 +284,18 @@ static void emit_rewrite_diff(const char *name_a,
copy_file_with_prefix(o->file, '+', two->data, two->size, new, reset);
}
+static const char *get_textconv(struct diff_filespec *one)
+{
+ if (!DIFF_FILE_VALID(one))
+ return NULL;
+ diff_filespec_load_driver(one);
+ return one->driver->textconv;
+}
+
static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
{
+ const char *textconv;
+
if (!DIFF_FILE_VALID(one)) {
mf->ptr = (char *)""; /* does not matter */
mf->size = 0;
@@ -294,10 +304,9 @@ static int fill_mmfile(mmfile_t *mf, struct diff_filespec *one)
else if (diff_populate_filespec(one, 0))
return -1;
- diff_filespec_load_driver(one);
- if (one->driver->textconv) {
+ if ((textconv = get_textconv(one))) {
size_t size;
- mf->ptr = run_textconv(one->driver->textconv, one, &size);
+ mf->ptr = run_textconv(textconv, one, &size);
if (!mf->ptr)
return -1;
mf->size = size;
@@ -1391,7 +1400,8 @@ static void builtin_diff(const char *name_a,
die("unable to read files to diff");
if (!DIFF_OPT_TST(o, TEXT) &&
- (diff_filespec_is_binary(one) || diff_filespec_is_binary(two))) {
+ ( (diff_filespec_is_binary(one) && !get_textconv(one)) ||
+ (diff_filespec_is_binary(two) && !get_textconv(two)) )) {
/* Quite common confusing case */
if (mf1.size == mf2.size &&
!memcmp(mf1.ptr, mf2.ptr, mf1.size))
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
index 1b09648..af94e1a 100755
--- a/t/t4030-diff-textconv.sh
+++ b/t/t4030-diff-textconv.sh
@@ -52,7 +52,7 @@ test_expect_success 'setup textconv filters' '
git config diff.fail.textconv false
'
-test_expect_failure 'diff produces text' '
+test_expect_success 'diff produces text' '
git diff HEAD^ HEAD >diff &&
find_diff <diff >actual &&
test_cmp expect.text actual
@@ -64,7 +64,7 @@ test_expect_success 'diff-tree produces binary' '
test_cmp expect.binary actual
'
-test_expect_failure 'log produces text' '
+test_expect_success 'log produces text' '
git log -1 -p >log &&
find_diff <log >actual &&
test_cmp expect.text actual
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 2/7] add userdiff textconv tests
From: Jeff King @ 2008-10-25 0:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
These tests provide a basic sanity check that textconv'd
files work. The tests try to describe how this configuration
_should_ work; thus some of the tests are marked to expect
failure.
In particular, we fail to actually textconv anything because
the 'diff.foo.binary' config option is not set, which will
be fixed in the next patch.
This also means that some "expect_failure" tests actually
seem to be fixed; in reality, this is just because textconv
is broken and its failure mode happens to make these tests
work.
Signed-off-by: Jeff King <peff@peff.net>
---
This brings in Johannes' typechange test, and adds a diffstat test
which we will fix along the way.
t/t4030-diff-textconv.sh | 118 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 118 insertions(+), 0 deletions(-)
create mode 100755 t/t4030-diff-textconv.sh
diff --git a/t/t4030-diff-textconv.sh b/t/t4030-diff-textconv.sh
new file mode 100755
index 0000000..1b09648
--- /dev/null
+++ b/t/t4030-diff-textconv.sh
@@ -0,0 +1,118 @@
+#!/bin/sh
+
+test_description='diff.*.textconv tests'
+. ./test-lib.sh
+
+find_diff() {
+ sed '1,/^index /d' | sed '/^-- $/,$d'
+}
+
+cat >expect.binary <<'EOF'
+Binary files a/file and b/file differ
+EOF
+
+cat >expect.text <<'EOF'
+--- a/file
++++ b/file
+@@ -1 +1,2 @@
+ 0
++1
+EOF
+
+cat >hexdump <<'EOF'
+#!/bin/sh
+perl -e '$/ = undef; $_ = <>; s/./ord($&)/ge; print $_' "$1"
+EOF
+chmod +x hexdump
+
+test_expect_success 'setup binary file with history' '
+ printf "\\0\\n" >file &&
+ git add file &&
+ git commit -m one &&
+ printf "\\1\\n" >>file &&
+ git add file &&
+ git commit -m two
+'
+
+test_expect_success 'file is considered binary by porcelain' '
+ git diff HEAD^ HEAD >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.binary actual
+'
+
+test_expect_success 'file is considered binary by plumbing' '
+ git diff-tree -p HEAD^ HEAD >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.binary actual
+'
+
+test_expect_success 'setup textconv filters' '
+ echo file diff=foo >.gitattributes &&
+ git config diff.foo.textconv "$PWD"/hexdump &&
+ git config diff.fail.textconv false
+'
+
+test_expect_failure 'diff produces text' '
+ git diff HEAD^ HEAD >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.text actual
+'
+
+test_expect_success 'diff-tree produces binary' '
+ git diff-tree -p HEAD^ HEAD >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.binary actual
+'
+
+test_expect_failure 'log produces text' '
+ git log -1 -p >log &&
+ find_diff <log >actual &&
+ test_cmp expect.text actual
+'
+
+test_expect_failure 'format-patch produces binary' '
+ git format-patch --no-binary --stdout HEAD^ >patch &&
+ find_diff <patch >actual &&
+ test_cmp expect.binary actual
+'
+
+cat >expect.stat <<'EOF'
+ file | Bin 2 -> 4 bytes
+ 1 files changed, 0 insertions(+), 0 deletions(-)
+EOF
+test_expect_failure 'diffstat does not run textconv' '
+ echo file diff=fail >.gitattributes &&
+ git diff --stat HEAD^ HEAD >actual &&
+ test_cmp expect.stat actual
+'
+# restore working setup
+echo file diff=foo >.gitattributes
+
+cat >expect.typechange <<'EOF'
+--- a/file
++++ /dev/null
+@@ -1,2 +0,0 @@
+-0
+-1
+diff --git a/file b/file
+new file mode 120000
+index ad8b3d2..67be421
+--- /dev/null
++++ b/file
+@@ -0,0 +1 @@
++frotz
+\ No newline at end of file
+EOF
+# make a symlink the hard way that works on symlink-challenged file systems
+test_expect_failure 'textconv does not act on symlinks' '
+ echo -n frotz > file &&
+ git add file &&
+ git ls-files -s | sed -e s/100644/120000/ |
+ git update-index --index-info &&
+ git commit -m typechange &&
+ git show >diff &&
+ find_diff <diff >actual &&
+ test_cmp expect.typechange actual
+'
+
+test_done
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
* [PATCH 1/7] diff: add missing static declaration
From: Jeff King @ 2008-10-25 0:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Matthieu Moy, git
In-Reply-To: <20081025004815.GA23851@coredump.intra.peff.net>
This function isn't used outside of diff.c; the 'static' was
simply overlooked in the original writing.
Signed-off-by: Jeff King <peff@peff.net>
---
Oops, it is actually 7 patches. This one is the same as before.
diff.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/diff.c b/diff.c
index e368fef..d1fd594 100644
--- a/diff.c
+++ b/diff.c
@@ -1282,7 +1282,7 @@ static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two)
emit_binary_diff_body(file, two, one);
}
-void diff_filespec_load_driver(struct diff_filespec *one)
+static void diff_filespec_load_driver(struct diff_filespec *one)
{
if (!one->driver)
one->driver = userdiff_find_by_path(one->path);
--
1.6.0.3.523.g38597.dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox