* Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff
From: Junio C Hamano @ 2009-03-22 0:03 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Sebastian Schuberth
In-Reply-To: <alpine.DEB.1.00.0903220053260.10279@pacific.mpi-cbg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Sat, 21 Mar 2009, Junio C Hamano wrote:
>
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>> > When preparing temporary files for an external diff, the files should
>> > be handled as if they were worktree files.
>>
>> I do not think so. convert_to_working_tree() aka "smudge" means you
>> would be feeding crap like $Id$ expansion to the external diff, which we
>> chose not to do quite on purpose.
>
> You might have missed me mentioning that we often can do without temporary
> files, taking the working directory copies right away?
>
> And if you think about it, it makes complete sense.
Not "complete".
What is at issue is how consistent the codepath that calls out to an
external diff should be with the rest of git that solely work with the
"clean" version of blob contents. If you value consistency, I would say
that it is valid to argue that letting it borrow from a work tree is a
bug. It should be always feeding a clean version.
But if you think that we do not care really about the correctness of the
external diff codepath, because it is a mechanism used mostly for giving
output to humans (as opposed to feeding the output of the external diff
program back to programs to be processed further) , I can understand why
you think it is easier to the external programs if "smudged" version is
fed to them.
Not just I can _understand_, but I think I could be pursuaded to agree
with that position, iff you try harder.
But the assumption/rationale behind this change needs to be spelled out.
In essence, we are sacrificing purity and correctness because we consider
ease of building external diff filter is more important.
Or something like that.
^ permalink raw reply
* [PATCH] git-branch: display "was sha1" on branch deletion rather than just "sha1"
From: Brandon Casey @ 2009-03-22 0:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
Make it more pleasant to read about a branch deletion by adding "was".
Jeff King suggested this, and I ignored it. He was right.
Update t3200 test again to match the change in output.
Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
builtin-branch.c | 2 +-
t/t3200-branch.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 14d4b91..07a440e 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -171,7 +171,7 @@ static int delete_branches(int argc, const char
**argv, int force, int kinds)
ret = 1;
} else {
struct strbuf buf = STRBUF_INIT;
- printf("Deleted %sbranch %s (%s).\n", remote,
+ printf("Deleted %sbranch %s (was %s).\n", remote,
bname.buf,
find_unique_abbrev(sha1, DEFAULT_ABBREV));
strbuf_addf(&buf, "branch.%s", bname.buf);
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 61a2010..1b1e9ec 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -195,7 +195,7 @@ test_expect_success 'test deleting branch deletes
branch config' \
test_expect_success 'test deleting branch without config' \
'git branch my7 s &&
sha1=$(git rev-parse my7 | cut -c 1-7) &&
- test "$(git branch -d my7 2>&1)" = "Deleted branch my7 ($sha1)."'
+ test "$(git branch -d my7 2>&1)" = "Deleted branch my7 (was $sha1)."'
test_expect_success 'test --track without .fetch entries' \
'git branch --track my8 &&
--
1.6.2.12.g83676
^ permalink raw reply related
* Are you Ok?
From: Davy @ 2009-03-22 0:57 UTC (permalink / raw)
To: git
Is it truth? http://ibf.breakingnewsfm.com/contact.php
^ permalink raw reply
* Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff
From: Junio C Hamano @ 2009-03-22 0:41 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Sebastian Schuberth, Jeff King
In-Reply-To: <7v8wmybf06.fsf@gitster.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Sat, 21 Mar 2009, Junio C Hamano wrote:
>>
>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>>
>>> > When preparing temporary files for an external diff, the files should
>>> > be handled as if they were worktree files.
>>>
>>> I do not think so. convert_to_working_tree() aka "smudge" means you
>>> would be feeding crap like $Id$ expansion to the external diff, which we
>>> chose not to do quite on purpose.
>>
>> You might have missed me mentioning that we often can do without temporary
>> files, taking the working directory copies right away?
>>
>> And if you think about it, it makes complete sense.
>
> Not "complete".
>
> What is at issue is how consistent the codepath that calls out to an
> external diff should be with the rest of git that solely work with the
> "clean" version of blob contents. If you value consistency, I would say
> that it is valid to argue that letting it borrow from a work tree is a
> bug. It should be always feeding a clean version.
>
> But if you think that we do not care really about the correctness of the
> external diff codepath, because it is a mechanism used mostly for giving
> output to humans (as opposed to feeding the output of the external diff
> program back to programs to be processed further) , I can understand why
> you think it is easier to the external programs if "smudged" version is
> fed to them.
>
> Not just I can _understand_, but I think I could be pursuaded to agree
> with that position, iff you try harder.
>
> But the assumption/rationale behind this change needs to be spelled out.
> In essence, we are sacrificing purity and correctness because we consider
> ease of building external diff filter is more important.
>
> Or something like that.
The situation is worse than I thought, and I am surely glad that this was
brought into my attention. I think diff.c::reuse_worktree_file() should
always say "Don't" when we know convert_to_working_tree() is not a no-op.
I have a suspicion that:
(1) borrowing from the work tree may not be buying us very much these
days; the introduction of the logic predates not just "clean/smudge"
feature but packfiles. Back then, the tradeoff was between opening a
loose object file, deflating and writing out a temporary and reusing
a file in the work tree. These days, most of the time the former
would be to reconstruct a blob from the pack data that is already
mapped, and performance characteristics would be different.
(2) having to check if convert_to_working_tree() is a no-op or not may be
a lot more costly than any performance gain we get from borrowing
from the work tree.
Here is a patch to get rid of the borrowing.
You can work on top of this patch to add the convert_to_working_tree()
call to prep_temp_blob(). Such a change would also affect the "textconv"
codepath and the result will start feeding smudged contents to the
"textconv" filter, and I think the argument "external tools prefer to be
fed smudged contents, not clean ones, because that is the representation
tailored for the platform" would hold even more stronger in that context.
diff.c | 69 ++-------------------------------------------------------------
1 files changed, 3 insertions(+), 66 deletions(-)
diff --git a/diff.c b/diff.c
index 75d9fab..4d71ebc 100644
--- a/diff.c
+++ b/diff.c
@@ -1746,67 +1746,6 @@ void fill_filespec(struct diff_filespec *spec, const unsigned char *sha1,
}
}
-/*
- * Given a name and sha1 pair, if the index tells us the file in
- * the work tree has that object contents, return true, so that
- * prepare_temp_file() does not have to inflate and extract.
- */
-static int reuse_worktree_file(const char *name, const unsigned char *sha1, int want_file)
-{
- struct cache_entry *ce;
- struct stat st;
- int pos, len;
-
- /* We do not read the cache ourselves here, because the
- * benchmark with my previous version that always reads cache
- * shows that it makes things worse for diff-tree comparing
- * two linux-2.6 kernel trees in an already checked out work
- * tree. This is because most diff-tree comparisons deal with
- * only a small number of files, while reading the cache is
- * expensive for a large project, and its cost outweighs the
- * savings we get by not inflating the object to a temporary
- * file. Practically, this code only helps when we are used
- * by diff-cache --cached, which does read the cache before
- * calling us.
- */
- if (!active_cache)
- return 0;
-
- /* We want to avoid the working directory if our caller
- * doesn't need the data in a normal file, this system
- * is rather slow with its stat/open/mmap/close syscalls,
- * and the object is contained in a pack file. The pack
- * is probably already open and will be faster to obtain
- * the data through than the working directory. Loose
- * objects however would tend to be slower as they need
- * to be individually opened and inflated.
- */
- if (!FAST_WORKING_DIRECTORY && !want_file && has_sha1_pack(sha1))
- return 0;
-
- len = strlen(name);
- pos = cache_name_pos(name, len);
- if (pos < 0)
- return 0;
- ce = active_cache[pos];
-
- /*
- * This is not the sha1 we are looking for, or
- * unreusable because it is not a regular file.
- */
- if (hashcmp(sha1, ce->sha1) || !S_ISREG(ce->ce_mode))
- return 0;
-
- /*
- * If ce matches the file in the work tree, we can reuse it.
- */
- if (ce_uptodate(ce) ||
- (!lstat(name, &st) && !ce_match_stat(ce, &st, 0)))
- return 1;
-
- return 0;
-}
-
static int populate_from_stdin(struct diff_filespec *s)
{
struct strbuf buf = STRBUF_INIT;
@@ -1861,8 +1800,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
if (S_ISGITLINK(s->mode))
return diff_populate_gitlink(s, size_only);
- if (!s->sha1_valid ||
- reuse_worktree_file(s->path, s->sha1, 0)) {
+ if (!s->sha1_valid) {
struct strbuf buf = STRBUF_INIT;
struct stat st;
int fd;
@@ -1988,8 +1926,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
remove_tempfile_installed = 1;
}
- if (!one->sha1_valid ||
- reuse_worktree_file(name, one->sha1, 1)) {
+ if (!one->sha1_valid) {
struct stat st;
if (lstat(name, &st) < 0) {
if (errno == ENOENT)
@@ -2011,7 +1948,7 @@ static struct diff_tempfile *prepare_temp_file(const char *name,
one->mode : S_IFLNK));
}
else {
- /* we can borrow from the file in the work tree */
+ /* we use the file in the work tree */
temp->name = name;
if (!one->sha1_valid)
strcpy(temp->hex, sha1_to_hex(null_sha1));
^ permalink raw reply related
* [PATCH 7/7 (v2)] checkout -: make "-" to mean "previous branch" everywhere
From: Junio C Hamano @ 2009-03-22 0:58 UTC (permalink / raw)
To: git
In-Reply-To: <1237673619-12608-8-git-send-email-gitster@pobox.com>
This is iffy, in that it teaches the very low level machinery to interpret
it as "the tip of the previous branch" when "-" is fed to it, and has a
high risk of unintended side effects.
This makes "git log ..-" to work as expected, which is marginally useful
because the revision parameter parser misinterprets the other direction
"git log -..". It also makes "git check-ref-format --branch -" to work,
which is not very useful because Porcelains can always ask for @{-1}.
It also makes a refname whose last component is "-" forbidden.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-checkout.c | 8 +++++---
refs.c | 3 +++
sha1_name.c | 21 +++++++++++++--------
3 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/builtin-checkout.c b/builtin-checkout.c
index 66df0c0..6b3b450 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -666,9 +666,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
arg = argv[0];
has_dash_dash = (argc > 1) && !strcmp(argv[1], "--");
- if (!strcmp(arg, "-"))
- arg = "@{-1}";
-
+ {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_branchname(&sb, arg);
+ arg = strbuf_detach(&sb, NULL);
+ }
if (get_sha1(arg, rev)) {
if (has_dash_dash) /* case (1) */
die("invalid reference: %s", arg);
diff --git a/refs.c b/refs.c
index e355489..7e27537 100644
--- a/refs.c
+++ b/refs.c
@@ -735,6 +735,9 @@ int check_ref_format(const char *ref)
if (!ch) {
if (ref <= cp - 2 && cp[-2] == '.')
return CHECK_REF_FORMAT_ERROR;
+ if (ref <= cp - 2 && cp[-2] == '-' &&
+ (cp - 3 < ref || cp[-3] == '/'))
+ return CHECK_REF_FORMAT_ERROR;
if (level < 2)
return CHECK_REF_FORMAT_ONELEVEL;
return ret;
diff --git a/sha1_name.c b/sha1_name.c
index 904bcd9..3972f4c 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -758,14 +758,19 @@ int interpret_branch_name(const char *name, struct strbuf *buf)
const char *brace;
char *num_end;
- if (name[0] != '@' || name[1] != '{' || name[2] != '-')
- return -1;
- brace = strchr(name, '}');
- if (!brace)
- return -1;
- nth = strtol(name+3, &num_end, 10);
- if (num_end != brace)
- return -1;
+ if (name[0] == '-' && !name[1]) {
+ nth = 1;
+ brace = name; /* "end of branch name expression" */
+ } else {
+ if (name[0] != '@' || name[1] != '{' || name[2] != '-')
+ return -1;
+ brace = strchr(name, '}');
+ if (!brace)
+ return -1;
+ nth = strtol(name+3, &num_end, 10);
+ if (num_end != brace)
+ return -1;
+ }
if (nth <= 0)
return -1;
cb.alloc = nth;
--
1.6.2.1.299.gda643a
^ permalink raw reply related
* Re: Disallow amending published commits?
From: Peter Harris @ 2009-03-22 1:53 UTC (permalink / raw)
To: James Pickens; +Cc: Git ML
In-Reply-To: <885649360903211549h751c19e6sbaa0e07a14413d19@mail.gmail.com>
On Sat, Mar 21, 2009 at 6:49 PM, James Pickens wrote:
> On Sat, Mar 21, 2009, Peter Harris <git@peter.is-a-geek.org> wrote:
>> Set receive.denyNonFastForwards if you don't want people to be able to
>> amend (or otherwise rewind) published history.
>
> Thanks, but unfortunately that won't work in our workflow. Users never
> push their changes; instead, they do a turnin to a continuous integration
> server. The server clones the central repo, pulls their changes into the
> clone, builds and tests it, then pushes to the central repo if it passes
> the tests. So integration happens via 'pull' instead of 'push'.
>
> We can't force the pulls to be fast forward only, because we need to allow
> turnins from multiple users to be built and tested in parallel, without
> requiring users to pull from each other or otherwise coordinate their
> turnins.
Okay. So in that workflow, you won't ever lose the original history.
If someone creates an alternate history that differs only slightly,
odds are your continuous integration server will get a merge conflict.
Presumably it will reject the pull request at that point.
If it doesn't conflict, you'll have both alternate histories. So
nothing is lost.
Maybe I'm misunderstanding the question? (That is definitely possible.
The idea that a person would go to the effort of rewriting history -
especially when that person knows the original history would stay put
- often enough to cause problems is like suggesting that a person
might write log messages in latin. I'm having a hard time envisioning
the need to write down a social rule about it, much less the need to
write an AI to try to detect it.)
Peter Harris
^ permalink raw reply
* Re: Disallow amending published commits?
From: Jeff King @ 2009-03-22 2:42 UTC (permalink / raw)
To: James Pickens; +Cc: Git ML
In-Reply-To: <885649360903211056u38ff6cabxbe1a17d57faaa0c4@mail.gmail.com>
On Sat, Mar 21, 2009 at 10:56:26AM -0700, James Pickens wrote:
> I wanted to have a pre-commit hook that would prevent users from
> amending a commit that had already been published, but I couldn't
> find any way in the pre-commit hook to figure out if --amend was
> used. Is there a way to do that? Or any better way to disallow
> amending published commits?
I don't think so; as somebody already mentioned, the usual time to
resolve such issues is at push-time. However, I can see how it would be
convenient to catch such a problem early, since by the time you push, it
may be much later and you don't remember exactly why you amended instead
of building on top (or as you indicated, your workflow may involve
pulling).
I suspect the right way to go about this is to inform the pre-commit
hook about the parents of the proposed commit. It already knows the
current branch (since it is in HEAD), and from there you should be able
to implement any policy logic regarding changing the shape of history
(including your request).
Right now that information is totally contained within the git-commit
process; probably the simplest thing would be to export a
space-separated list of SHA-1's to the hook.
-Peff
^ permalink raw reply
* Re: Disallow amending published commits?
From: Peter Harris @ 2009-03-22 2:57 UTC (permalink / raw)
To: James Pickens; +Cc: Git ML
In-Reply-To: <eaa105840903211853p65327ffdvebbe28da5f256871@mail.gmail.com>
On Sat, Mar 21, 2009 at 9:53 PM, Peter Harris wrote:
> On Sat, Mar 21, 2009 at 6:49 PM, James Pickens wrote:
>> On Sat, Mar 21, 2009, Peter Harris <git@peter.is-a-geek.org> wrote:
>>> Set receive.denyNonFastForwards if you don't want people to be able to
>>> amend (or otherwise rewind) published history.
>>
>> Thanks, but unfortunately that won't work in our workflow. Users never
>> push their changes; instead, they do a turnin to a continuous integration
>> server. The server clones the central repo, pulls their changes into the
>> clone, builds and tests it, then pushes to the central repo if it passes
>> the tests. So integration happens via 'pull' instead of 'push'.
>>
>> We can't force the pulls to be fast forward only, because we need to allow
>> turnins from multiple users to be built and tested in parallel, without
>> requiring users to pull from each other or otherwise coordinate their
>> turnins.
>
> Okay. So in that workflow, you won't ever lose the original history.
(Replying to myself, since I thought of one other thing)
You could, if you wanted, 'pull' into a clean branch. Ensure that it
was a fast-forward, and only then merge the result into the
integration branch. Developers would have to sync-up with the central
repo, but at least they wouldn't have to sync with each other.
Peter Harris
^ permalink raw reply
* Re: How to go to git from svn without checkout
From: Jeff King @ 2009-03-22 3:20 UTC (permalink / raw)
To: Mercedes6s; +Cc: git
In-Reply-To: <22640020.post@talk.nabble.com>
On Sat, Mar 21, 2009 at 01:25:38PM -0700, Mercedes6s wrote:
> Hello. I've been falling more and more in love with git and I want to move
> our biggest project because it is my biggest pain over to git to make our
> lives easier. The problem is our project is about 50 gigs (A lot of media
> files involved). Also, we got developers all over the world that are
> running on slow connections. Some took a week to get the project up and
> running in the first place. Luckily, they are only isolated instances.
> What I'm wondering is can I have all of them turn their projects into git
> repositories locally and have all of them sync with a master repository once
> they are done. The actual project is only about 500 megs and the changes
> are probably only a few K since they keep their projects up to date, so
> that's more acceptable, but I don't want them to bring down all those media
> files again and I doubt they will be willing to do it. Is this possible,
> and how would I do it?
I'm not sure you can. "git svn clone" will give a stable commit ID in
git; that is, two runs of the same import will yield interoperable git
repositories. However, I'm not sure if that is actually useful to you;
the people with svn checkouts don't _have_ the history. So the clone
operation will have to talk to the server.
However, you should really consider whether you want those 50G in the
git repository at all. Git is pretty good at not looking at parts of the
repository that aren't necessary to an operation, but whole-repository
operations like packing and cloning are going to be absurdly slow.
Are those files actually changing? Would it be feasible to put the
"main" part of the project in git and just include something like
symlinks in the repository pointing to your media? Then each local
developer could clone the git project and just move their existing 50G
of media files into place.
-Peff
^ permalink raw reply
* Re: Disallow amending published commits?
From: James Pickens @ 2009-03-22 4:09 UTC (permalink / raw)
To: Peter Harris; +Cc: Git ML
In-Reply-To: <eaa105840903211853p65327ffdvebbe28da5f256871@mail.gmail.com>
On Sat, Mar 21, 2009, Peter Harris <git@peter.is-a-geek.org> wrote:
> Okay. So in that workflow, you won't ever lose the original history.
>
> If someone creates an alternate history that differs only slightly,
> odds are your continuous integration server will get a merge conflict.
> Presumably it will reject the pull request at that point.
>
> If it doesn't conflict, you'll have both alternate histories. So
> nothing is lost.
>
> Maybe I'm misunderstanding the question? (That is definitely possible.
> The idea that a person would go to the effort of rewriting history -
> especially when that person knows the original history would stay put
> - often enough to cause problems is like suggesting that a person
> might write log messages in latin. I'm having a hard time envisioning
> the need to write down a social rule about it, much less the need to
> write an AI to try to detect it.)
I think you understood the question perfectly, and your comments all make
sense. Perhaps I'm just being paranoid and this won't be a problem at all.
A bit of background might help explain my paranoia: I'm about to pilot Git
on a fairly large project, where none of the users have Git experience, and
many of them don't have much experience with any other version control
system either. I had to fight hard to get this pilot approved, and a lot
of people will be watching to see how it goes, so I'm trying to do anything
I can to make sure it will be successful.
James
^ permalink raw reply
* [PATCHv2 0/3] format-patch --attach/--inline use filename instead of SHA1
From: Stephen Boyd @ 2009-03-22 4:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
I'm resending this series because it seems it wasn't picked up probably because
the patches were mangled by my mailer.
This patch series modifies the behavior of format-patch when used with the
--attach or --inline command line settings. Current behavior names the attached
or inlined patches with the SHA1 of the commit, which isn't very friendly or
easy for a human to use when downloading the attachments. This series replaces
the SHA1 values with the filename used by format-patch when it outputs the
patches to files.
Stephen Boyd (3):
format-patch: create patch filename in one function
format-patch: --attach/inline uses filename instead of SHA1
format-patch: --numbered-files and --stdout aren't mutually exclusive
Documentation/git-format-patch.txt | 1 -
builtin-log.c | 51 ++++++++--------
log-tree.c | 8 +-
revision.h | 1 +
t/t4013-diff-various.sh | 1 +
..._--attach_--stdout_--suffix=.diff_initial..side | 61 ++++++++++++++++++++
....format-patch_--attach_--stdout_initial..master | 12 ++--
...format-patch_--attach_--stdout_initial..master^ | 8 +-
...ff.format-patch_--attach_--stdout_initial..side | 4 +-
...tdout_--subject-prefix=TESTCASE_initial..master | 12 ++--
....format-patch_--inline_--stdout_initial..master | 12 ++--
...format-patch_--inline_--stdout_initial..master^ | 8 +-
...ormat-patch_--inline_--stdout_initial..master^^ | 4 +-
...ff.format-patch_--inline_--stdout_initial..side | 4 +-
14 files changed, 124 insertions(+), 63 deletions(-)
create mode 100644 t/t4013/diff.format-patch_--attach_--stdout_--suffix=.diff_initial..side
^ permalink raw reply
* [PATCHv2 1/3] format-patch: create patch filename in one function
From: Stephen Boyd @ 2009-03-22 4:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <1237696363-6819-1-git-send-email-bebarino@gmail.com>
reopen_stdout() usually takes the oneline description of a commit,
appends the patch suffix, prepends the output directory (if any) and
then reopens stdout as the resulting file. Now the patch filename (the
oneline description and the patch suffix) is created in
get_patch_filename() and passed to reopen_stdout() which prepends the
output directory and reopens stdout as that file.
The original function to get the oneline description,
get_oneline_for_filename(), has been renamed to get_patch_filename() to
reflect its new functionality.
Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
builtin-log.c | 49 +++++++++++++++++++++++++------------------------
1 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/builtin-log.c b/builtin-log.c
index 8af55d2..cc6d663 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -519,21 +519,17 @@ static int git_format_config(const char *var, const char *value, void *cb)
}
-static const char *get_oneline_for_filename(struct commit *commit,
- int keep_subject)
+static const char *get_patch_filename(char* sol, int keep_subject, int nr)
{
static char filename[PATH_MAX];
- char *sol;
int len = 0;
int suffix_len = strlen(fmt_patch_suffix) + 1;
- sol = strstr(commit->buffer, "\n\n");
- if (!sol)
- filename[0] = '\0';
- else {
+ if (sol)
+ {
int j, space = 0;
- sol += 2;
+ len += sprintf(filename, "%04d-", nr);
/* strip [PATCH] or [PATCH blabla] */
if (!keep_subject && !prefixcmp(sol, "[PATCH")) {
char *eos = strchr(sol + 6, ']');
@@ -564,8 +560,11 @@ static const char *get_oneline_for_filename(struct commit *commit,
while (filename[len - 1] == '.'
|| filename[len - 1] == '-')
len--;
- filename[len] = '\0';
+ strcpy(filename + len, fmt_patch_suffix);
}
+ else
+ sprintf(filename, "%d", nr);
+
return filename;
}
@@ -573,7 +572,7 @@ static FILE *realstdout = NULL;
static const char *output_directory = NULL;
static int outdir_offset;
-static int reopen_stdout(const char *oneline, int nr, struct rev_info *rev)
+static int reopen_stdout(const char *oneline, struct rev_info *rev)
{
char filename[PATH_MAX];
int len = 0;
@@ -588,15 +587,7 @@ static int reopen_stdout(const char *oneline, int nr, struct rev_info *rev)
if (filename[len - 1] != '/')
filename[len++] = '/';
}
-
- if (!oneline)
- len += sprintf(filename + len, "%d", nr);
- else {
- len += sprintf(filename + len, "%04d-", nr);
- len += snprintf(filename + len, sizeof(filename) - len - 1
- - suffix_len, "%s", oneline);
- strcpy(filename + len, fmt_patch_suffix);
- }
+ strncpy(filename+len, oneline, PATH_MAX-len-1);
if (!DIFF_OPT_TST(&rev->diffopt, QUIET))
fprintf(realstdout, "%s\n", filename + outdir_offset);
@@ -688,8 +679,9 @@ static void make_cover_letter(struct rev_info *rev, int use_stdout,
if (rev->commit_format != CMIT_FMT_EMAIL)
die("Cover letter needs email format");
- if (!use_stdout && reopen_stdout(numbered_files ?
- NULL : "cover-letter", 0, rev))
+ if (!use_stdout && reopen_stdout(get_patch_filename(numbered_files ?
+ NULL : "cover-letter",
+ 0, 0), rev))
return;
head_sha1 = sha1_to_hex(head->object.sha1);
@@ -802,6 +794,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
struct patch_ids ids;
char *add_signoff = NULL;
struct strbuf buf = STRBUF_INIT;
+ char *sol = NULL;
git_config(git_format_config, NULL);
init_revisions(&rev, prefix);
@@ -1106,9 +1099,17 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
}
gen_message_id(&rev, sha1_to_hex(commit->object.sha1));
}
- if (!use_stdout && reopen_stdout(numbered_files ? NULL :
- get_oneline_for_filename(commit, keep_subject),
- rev.nr, &rev))
+ /*
+ * If we're outputting numbered files we don't need a subject
+ * line. Otherwise we want the subject line of the commit
+ * message which starts after the first double newline
+ * occurence in the commit buffer.
+ */
+ if (!numbered_files && (sol = strstr(commit->buffer, "\n\n")))
+ sol += 2;
+ if (!use_stdout && reopen_stdout(get_patch_filename(sol,
+ keep_subject,
+ rev.nr), &rev))
die("Failed to create output files");
shown = log_tree_commit(&rev, commit);
free(commit->buffer);
--
1.6.2
^ permalink raw reply related
* [PATCHv2 2/3] format-patch: --attach/inline uses filename instead of SHA1
From: Stephen Boyd @ 2009-03-22 4:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <1237696363-6819-2-git-send-email-bebarino@gmail.com>
Currently when format-patch is used with --attach or --inline the patch
attachment has the SHA1 of the commit for its filename. This replaces
the SHA1 with the filename used by format-patch when outputting to files.
Fix tests relying on the SHA1 output and add a test showing how the
--suffix option affects the attachment filename output.
Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
Is modifying the rev_info struct correct?
builtin-log.c | 6 +-
log-tree.c | 8 +-
revision.h | 1 +
t/t4013-diff-various.sh | 1 +
..._--attach_--stdout_--suffix=.diff_initial..side | 61 ++++++++++++++++++++
....format-patch_--attach_--stdout_initial..master | 12 ++--
...format-patch_--attach_--stdout_initial..master^ | 8 +-
...ff.format-patch_--attach_--stdout_initial..side | 4 +-
...tdout_--subject-prefix=TESTCASE_initial..master | 12 ++--
....format-patch_--inline_--stdout_initial..master | 12 ++--
...format-patch_--inline_--stdout_initial..master^ | 8 +-
...ormat-patch_--inline_--stdout_initial..master^^ | 4 +-
...ff.format-patch_--inline_--stdout_initial..side | 4 +-
13 files changed, 102 insertions(+), 39 deletions(-)
create mode 100644 t/t4013/diff.format-patch_--attach_--stdout_--suffix=.diff_initial..side
diff --git a/builtin-log.c b/builtin-log.c
index cc6d663..8ea25e0 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -1107,9 +1107,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
*/
if (!numbered_files && (sol = strstr(commit->buffer, "\n\n")))
sol += 2;
- if (!use_stdout && reopen_stdout(get_patch_filename(sol,
- keep_subject,
- rev.nr), &rev))
+ rev.fmt_patch_filename = get_patch_filename(sol, keep_subject,
+ rev.nr);
+ if (!use_stdout && reopen_stdout(rev.fmt_patch_filename, &rev))
die("Failed to create output files");
shown = log_tree_commit(&rev, commit);
free(commit->buffer);
diff --git a/log-tree.c b/log-tree.c
index 9565c18..dc37d16 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -245,14 +245,14 @@ void log_write_email_headers(struct rev_info *opt, const char *name,
snprintf(buffer, sizeof(buffer) - 1,
"\n--%s%s\n"
"Content-Type: text/x-patch;"
- " name=\"%s.diff\"\n"
+ " name=\"%s\"\n"
"Content-Transfer-Encoding: 8bit\n"
"Content-Disposition: %s;"
- " filename=\"%s.diff\"\n\n",
+ " filename=\"%s\"\n\n",
mime_boundary_leader, opt->mime_boundary,
- name,
+ opt->fmt_patch_filename,
opt->no_inline ? "attachment" : "inline",
- name);
+ opt->fmt_patch_filename);
opt->diffopt.stat_sep = buffer;
}
*subject_p = subject;
diff --git a/revision.h b/revision.h
index ad123d7..c34eac9 100644
--- a/revision.h
+++ b/revision.h
@@ -86,6 +86,7 @@ struct rev_info {
struct log_info *loginfo;
int nr, total;
const char *mime_boundary;
+ const char *fmt_patch_filename;
char *message_id;
struct string_list *ref_message_ids;
const char *add_signoff;
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 426e64e..6592a4f 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -246,6 +246,7 @@ format-patch --stdout initial..master
format-patch --stdout --no-numbered initial..master
format-patch --stdout --numbered initial..master
format-patch --attach --stdout initial..side
+format-patch --attach --stdout --suffix=.diff initial..side
format-patch --attach --stdout initial..master^
format-patch --attach --stdout initial..master
format-patch --inline --stdout initial..side
diff --git a/t/t4013/diff.format-patch_--attach_--stdout_--suffix=.diff_initial..side b/t/t4013/diff.format-patch_--attach_--stdout_--suffix=.diff_initial..side
new file mode 100644
index 0000000..52116d3
--- /dev/null
+++ b/t/t4013/diff.format-patch_--attach_--stdout_--suffix=.diff_initial..side
@@ -0,0 +1,61 @@
+$ git format-patch --attach --stdout --suffix=.diff initial..side
+From c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a Mon Sep 17 00:00:00 2001
+From: A U Thor <author@example.com>
+Date: Mon, 26 Jun 2006 00:03:00 +0000
+Subject: [PATCH] Side
+MIME-Version: 1.0
+Content-Type: multipart/mixed; boundary="------------g-i-t--v-e-r-s-i-o-n"
+
+This is a multi-part message in MIME format.
+--------------g-i-t--v-e-r-s-i-o-n
+Content-Type: text/plain; charset=UTF-8; format=fixed
+Content-Transfer-Encoding: 8bit
+
+---
+ dir/sub | 2 ++
+ file0 | 3 +++
+ file3 | 4 ++++
+ 3 files changed, 9 insertions(+), 0 deletions(-)
+ create mode 100644 file3
+
+
+--------------g-i-t--v-e-r-s-i-o-n
+Content-Type: text/x-patch; name="0001-Side.diff"
+Content-Transfer-Encoding: 8bit
+Content-Disposition: attachment; filename="0001-Side.diff"
+
+diff --git a/dir/sub b/dir/sub
+index 35d242b..7289e35 100644
+--- a/dir/sub
++++ b/dir/sub
+@@ -1,2 +1,4 @@
+ A
+ B
++1
++2
+diff --git a/file0 b/file0
+index 01e79c3..f4615da 100644
+--- a/file0
++++ b/file0
+@@ -1,3 +1,6 @@
+ 1
+ 2
+ 3
++A
++B
++C
+diff --git a/file3 b/file3
+new file mode 100644
+index 0000000..7289e35
+--- /dev/null
++++ b/file3
+@@ -0,0 +1,4 @@
++A
++B
++1
++2
+
+--------------g-i-t--v-e-r-s-i-o-n--
+
+
+$
diff --git a/t/t4013/diff.format-patch_--attach_--stdout_initial..master b/t/t4013/diff.format-patch_--attach_--stdout_initial..master
index e5ab744..ce49bd6 100644
--- a/t/t4013/diff.format-patch_--attach_--stdout_initial..master
+++ b/t/t4013/diff.format-patch_--attach_--stdout_initial..master
@@ -22,9 +22,9 @@ This is the second commit.
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Type: text/x-patch; name="0001-Second.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: attachment; filename="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Disposition: attachment; filename="0001-Second.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..8422d40 100644
@@ -80,9 +80,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Type: text/x-patch; name="0002-Third.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: attachment; filename="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Disposition: attachment; filename="0002-Third.patch"
diff --git a/dir/sub b/dir/sub
index 8422d40..cead32e 100644
@@ -129,9 +129,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Type: text/x-patch; name="0003-Side.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: attachment; filename="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Disposition: attachment; filename="0003-Side.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..7289e35 100644
diff --git a/t/t4013/diff.format-patch_--attach_--stdout_initial..master^ b/t/t4013/diff.format-patch_--attach_--stdout_initial..master^
index 2c71d20..5f1b238 100644
--- a/t/t4013/diff.format-patch_--attach_--stdout_initial..master^
+++ b/t/t4013/diff.format-patch_--attach_--stdout_initial..master^
@@ -22,9 +22,9 @@ This is the second commit.
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Type: text/x-patch; name="0001-Second.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: attachment; filename="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Disposition: attachment; filename="0001-Second.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..8422d40 100644
@@ -80,9 +80,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Type: text/x-patch; name="0002-Third.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: attachment; filename="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Disposition: attachment; filename="0002-Third.patch"
diff --git a/dir/sub b/dir/sub
index 8422d40..cead32e 100644
diff --git a/t/t4013/diff.format-patch_--attach_--stdout_initial..side b/t/t4013/diff.format-patch_--attach_--stdout_initial..side
index 38f7902..4a2364a 100644
--- a/t/t4013/diff.format-patch_--attach_--stdout_initial..side
+++ b/t/t4013/diff.format-patch_--attach_--stdout_initial..side
@@ -20,9 +20,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Type: text/x-patch; name="0001-Side.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: attachment; filename="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Disposition: attachment; filename="0001-Side.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..7289e35 100644
diff --git a/t/t4013/diff.format-patch_--inline_--stdout_--subject-prefix=TESTCASE_initial..master b/t/t4013/diff.format-patch_--inline_--stdout_--subject-prefix=TESTCASE_initial..master
index 58f8a7b..ca3f60b 100644
--- a/t/t4013/diff.format-patch_--inline_--stdout_--subject-prefix=TESTCASE_initial..master
+++ b/t/t4013/diff.format-patch_--inline_--stdout_--subject-prefix=TESTCASE_initial..master
@@ -22,9 +22,9 @@ This is the second commit.
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Type: text/x-patch; name="0001-Second.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Disposition: inline; filename="0001-Second.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..8422d40 100644
@@ -80,9 +80,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Type: text/x-patch; name="0002-Third.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Disposition: inline; filename="0002-Third.patch"
diff --git a/dir/sub b/dir/sub
index 8422d40..cead32e 100644
@@ -129,9 +129,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Type: text/x-patch; name="0003-Side.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Disposition: inline; filename="0003-Side.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..7289e35 100644
diff --git a/t/t4013/diff.format-patch_--inline_--stdout_initial..master b/t/t4013/diff.format-patch_--inline_--stdout_initial..master
index 9e7bbdf..08f2301 100644
--- a/t/t4013/diff.format-patch_--inline_--stdout_initial..master
+++ b/t/t4013/diff.format-patch_--inline_--stdout_initial..master
@@ -22,9 +22,9 @@ This is the second commit.
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Type: text/x-patch; name="0001-Second.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Disposition: inline; filename="0001-Second.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..8422d40 100644
@@ -80,9 +80,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Type: text/x-patch; name="0002-Third.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Disposition: inline; filename="0002-Third.patch"
diff --git a/dir/sub b/dir/sub
index 8422d40..cead32e 100644
@@ -129,9 +129,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Type: text/x-patch; name="0003-Side.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Disposition: inline; filename="0003-Side.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..7289e35 100644
diff --git a/t/t4013/diff.format-patch_--inline_--stdout_initial..master^ b/t/t4013/diff.format-patch_--inline_--stdout_initial..master^
index f881f64..07f1230 100644
--- a/t/t4013/diff.format-patch_--inline_--stdout_initial..master^
+++ b/t/t4013/diff.format-patch_--inline_--stdout_initial..master^
@@ -22,9 +22,9 @@ This is the second commit.
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Type: text/x-patch; name="0001-Second.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Disposition: inline; filename="0001-Second.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..8422d40 100644
@@ -80,9 +80,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Type: text/x-patch; name="0002-Third.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0.diff"
+Content-Disposition: inline; filename="0002-Third.patch"
diff --git a/dir/sub b/dir/sub
index 8422d40..cead32e 100644
diff --git a/t/t4013/diff.format-patch_--inline_--stdout_initial..master^^ b/t/t4013/diff.format-patch_--inline_--stdout_initial..master^^
index 4f258b8..29e00ab 100644
--- a/t/t4013/diff.format-patch_--inline_--stdout_initial..master^^
+++ b/t/t4013/diff.format-patch_--inline_--stdout_initial..master^^
@@ -22,9 +22,9 @@ This is the second commit.
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Type: text/x-patch; name="0001-Second.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="1bde4ae5f36c8d9abe3a0fce0c6aab3c4a12fe44.diff"
+Content-Disposition: inline; filename="0001-Second.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..8422d40 100644
diff --git a/t/t4013/diff.format-patch_--inline_--stdout_initial..side b/t/t4013/diff.format-patch_--inline_--stdout_initial..side
index e86dce6..67633d4 100644
--- a/t/t4013/diff.format-patch_--inline_--stdout_initial..side
+++ b/t/t4013/diff.format-patch_--inline_--stdout_initial..side
@@ -20,9 +20,9 @@ Content-Transfer-Encoding: 8bit
--------------g-i-t--v-e-r-s-i-o-n
-Content-Type: text/x-patch; name="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Type: text/x-patch; name="0001-Side.patch"
Content-Transfer-Encoding: 8bit
-Content-Disposition: inline; filename="c7a2ab9e8eac7b117442a607d5a9b3950ae34d5a.diff"
+Content-Disposition: inline; filename="0001-Side.patch"
diff --git a/dir/sub b/dir/sub
index 35d242b..7289e35 100644
--
1.6.2
^ permalink raw reply related
* [PATCHv2 3/3] format-patch: --numbered-files and --stdout aren't mutually exclusive
From: Stephen Boyd @ 2009-03-22 4:32 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <1237696363-6819-3-git-send-email-bebarino@gmail.com>
For example:
git format-patch --numbered-files --stdout --attach HEAD~~
will create two messages with files 1 and 2 attached respectively.
There is no effect when using --numbered-files and --stdout together
without an --attach or --inline, the files simply aren't created.
Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
Documentation/git-format-patch.txt | 1 -
builtin-log.c | 2 --
2 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index c14e3ee..c2eb5fa 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -97,7 +97,6 @@ include::diff-options.txt[]
--numbered-files::
Output file names will be a simple number sequence
without the default first line of the commit appended.
- Mutually exclusive with the --stdout option.
-k::
--keep-subject::
diff --git a/builtin-log.c b/builtin-log.c
index 8ea25e0..f81d3a3 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -951,8 +951,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
die ("-n and -k are mutually exclusive.");
if (keep_subject && subject_prefix)
die ("--subject-prefix and -k are mutually exclusive.");
- if (numbered_files && use_stdout)
- die ("--numbered-files and --stdout are mutually exclusive.");
argc = setup_revisions(argc, argv, &rev, "HEAD");
if (argc > 1)
--
1.6.2
^ permalink raw reply related
* Re: [PATCH v2 2/2] pack-objects: don't loosen objects available in alternate or kept packs
From: Junio C Hamano @ 2009-03-22 4:43 UTC (permalink / raw)
To: Brandon Casey; +Cc: git
In-Reply-To: <ee63ef30903211526n47c40052mc40dc018f25c99fd@mail.gmail.com>
Brandon Casey <drafnel@gmail.com> writes:
> If pack-objects is called with the --unpack-unreachable option then it will
> unpack (i.e. loosen) all unreferenced objects from local not-kept packs,
> including those that also exist in packs residing in an alternate object
> database or a local kept pack. The primary(sole?) user of this option is
> git-repack. In this case, repack will follow the call to pack-objects with
> a call to prune-packed which will delete these newly loosened objects,
> making the act of loosening a waste of time. The unnecessary loosening can
> be avoided by checking whether an object exists in a non-local pack or a
> local kept pack before loosening it.
>
> This fixes the 'local packed unreachable obs that exist in alternate ODB
> are not loosened' test in t7700.
>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
Thanks.
Both patches are whitespace damaged, but I can cope. But I am not sure
about one thing...
> builtin-pack-objects.c | 26 +++++++++++++++++++++++++-
> t/t7700-repack.sh | 2 +-
> 2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 6222f19..3f477c5 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -1944,6 +1944,29 @@ static void
> add_objects_in_unpacked_packs(struct rev_info *revs)
> free(in_pack.array);
> }
>
> +static int has_sha1_pack_kept_or_nonlocal(const unsigned char *sha1)
> +{
> + static struct packed_git *last_found = (void *)1;
> + struct packed_git *p;
> +
> + p = (last_found == (void *)1) ? packed_git : last_found;
Why (void *)1, not like:
static struct packed_git *last_found;
struct packed_git *p = last_found ? last_found : packed_git;
Am I missing something?
> + while (p) {
> + if ((!p->pack_local || p->pack_keep) &&
> + find_pack_entry_one(sha1, p)) {
> + last_found = p;
> + return 1;
> + }
> + if (p == last_found)
> + p = packed_git;
> + else
> + p = p->next;
> + if (p == last_found)
> + p = p->next;
> + }
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCHv2 1/3] format-patch: create patch filename in one function
From: Junio C Hamano @ 2009-03-22 5:31 UTC (permalink / raw)
To: Stephen Boyd; +Cc: git
In-Reply-To: <1237696363-6819-2-git-send-email-bebarino@gmail.com>
Stephen Boyd <bebarino@gmail.com> writes:
> reopen_stdout() usually takes the oneline description of a commit,
> appends the patch suffix, prepends the output directory (if any) and
> then reopens stdout as the resulting file. Now the patch filename (the
> oneline description and the patch suffix) is created in
> get_patch_filename() and passed to reopen_stdout() which prepends the
> output directory and reopens stdout as that file.
The renaming is a good idea even without any change in the feature.
Naming functions after what their result is used _for_ is never a good
idea, and we should name them after what they do.
Does it still make sense to pass "keep_subject" to the function? After
all what it does is to retain "[PATCH.." prefix that is useless for the
purpose of making each patch easily identifiable. Because people almost
always use patch acceptance tools in non-keep mode to strip the
"[PATCH..]" prefix when creating the commits these days anyway, it may
make more sense to lose the parameter altogether and simplify the
processing.
> -static const char *get_oneline_for_filename(struct commit *commit,
> - int keep_subject)
> +static const char *get_patch_filename(char* sol, int keep_subject, int nr)
Asterisk sticks to the variable name, not type name.
I also wonder if it makes sense to move what this function does into a
user format; especially the logic that sanitizes the oneline string into
filename friendly one may be something Porcelains may want an access to
from outside.
IOW, you can introduce a new format specifier (say, "%f") to
format_commit_message() and the implemention of get_patch_filename() would
just prepare a strbuf and call format_commit_message() on it, no?
^ permalink raw reply
* Re: [PATCHv2 2/3] format-patch: --attach/inline uses filename instead of SHA1
From: Junio C Hamano @ 2009-03-22 5:36 UTC (permalink / raw)
To: Stephen Boyd; +Cc: git
In-Reply-To: <1237696363-6819-3-git-send-email-bebarino@gmail.com>
Stephen Boyd <bebarino@gmail.com> writes:
> Currently when format-patch is used with --attach or --inline the patch
> attachment has the SHA1 of the commit for its filename. This replaces
> the SHA1 with the filename used by format-patch when outputting to files.
>
> Fix tests relying on the SHA1 output and add a test showing how the
> --suffix option affects the attachment filename output.
>
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
> ---
> Is modifying the rev_info struct correct?
Most of the rev_info is about the overall traversal not about individual
commits, and I think the new field you are adding is about a single commit
you will update every time you switch to a new commit to process, so in
that sense it may violate the general idea of what rev-info is, but I do
not think it matters too much.
^ permalink raw reply
* Re: [PATCHv2 3/3] format-patch: --numbered-files and --stdout aren't mutually exclusive
From: Junio C Hamano @ 2009-03-22 5:44 UTC (permalink / raw)
To: Stephen Boyd; +Cc: git
In-Reply-To: <1237696363-6819-4-git-send-email-bebarino@gmail.com>
Stephen Boyd <bebarino@gmail.com> writes:
> For example:
>
> git format-patch --numbered-files --stdout --attach HEAD~~
>
> will create two messages with files 1 and 2 attached respectively.
> There is no effect when using --numbered-files and --stdout together
> without an --attach or --inline, the files simply aren't created.
Hmm, "the files aren't created" is true, but I had to scratch my head if
you are describing a bug or justifying the change by saying "so filename
does not matter".
I think the change makes sense. It could be argued that combination of
these two options should give a warning("ignoring --numbered"), but I do
not think it is even worth it.
^ permalink raw reply
* Re: [PATCHv2 1/3] format-patch: create patch filename in one function
From: Stephen Boyd @ 2009-03-22 5:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vwsai86nw.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> The renaming is a good idea even without any change in the feature.
> Naming functions after what their result is used _for_ is never a good
> idea, and we should name them after what they do.
>
> Does it still make sense to pass "keep_subject" to the function? After
> all what it does is to retain "[PATCH.." prefix that is useless for the
> purpose of making each patch easily identifiable. Because people almost
> always use patch acceptance tools in non-keep mode to strip the
> "[PATCH..]" prefix when creating the commits these days anyway, it may
> make more sense to lose the parameter altogether and simplify the
> processing.
I originally removed "keep_subject" but I took it out because I couldn't
figure out what it was being used for and I thought it might be
important. When it's removed all the tests pass. It's not even used by
other code besides a few if cases regarding numbering logic so I think
the entire command line switch could be removed.
>
> I also wonder if it makes sense to move what this function does into a
> user format; especially the logic that sanitizes the oneline string into
> filename friendly one may be something Porcelains may want an access to
> from outside.
>
> IOW, you can introduce a new format specifier (say, "%f") to
> format_commit_message() and the implemention of get_patch_filename() would
> just prepare a strbuf and call format_commit_message() on it, no?
This sounds great! I'm new so I don't know where to look for something
like this.
^ permalink raw reply
* Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff
From: Jeff King @ 2009-03-22 6:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git, Sebastian Schuberth
In-Reply-To: <7vzlfe9ynj.fsf@gitster.siamese.dyndns.org>
On Sat, Mar 21, 2009 at 05:41:52PM -0700, Junio C Hamano wrote:
> I have a suspicion that:
>
> (1) borrowing from the work tree may not be buying us very much these
> days; the introduction of the logic predates not just "clean/smudge"
> feature but packfiles. Back then, the tradeoff was between opening a
> loose object file, deflating and writing out a temporary and reusing
> a file in the work tree. These days, most of the time the former
> would be to reconstruct a blob from the pack data that is already
> mapped, and performance characteristics would be different.
I don't think this is the case. We only reuse the worktree file when we
have already read the index, which means the main user is "git diff
--cached". If a file's SHA1 is the same in the HEAD and the index, then
we don't need to run the diff at all. But if it is, then that implies
content newly added for a commit, which is probably not in a packfile,
but rather a loose object.
Try something like this with and without your patch:
cd linux-2.6 &&
git ls-files |
while read f; do
echo trivial change >>$f
done
git add -u
git diff --cached
I get about 10.5s with stock git, and 13.5s with your patch (actually,
the first run with without reuse_worktree_file is doubly painful -- it
touches the disk for all of the newly written loose objects).
So I think it does make a difference now. The question, though, is:
> (2) having to check if convert_to_working_tree() is a no-op or not may be
> a lot more costly than any performance gain we get from borrowing
> from the work tree.
...how expensive is the check for convert_to_working_tree? It should
just be a gitattributes lookup, shouldn't it? Which:
a. we are doing anyway and caching
b. which takes a fraction of a second (try "time git ls-files | git
check-attr --stdin diff >/dev/null", which should give a
worst-case).
To be fair, though, the diff I outlined above is pretty ridiculous.
Worktree reuse saved me three seconds, but out of 26683 files. Even if
you had 1000 files in your commit, that's still only .1s.
Where I think it _would_ make an impact is in crazy large-file
repositories. Where the I/O cost of dumping a 200MB avi into a tempfile
is actually meaningful. But for that, even the current code is not very
good; it is optimized for many small files. For a few large files, you
are probably better off reading the index from disk in order to make
that optimization for other cases (like tree to tree diff, where one of
the trees happens to have the same contents for the file as HEAD).
> You can work on top of this patch to add the convert_to_working_tree()
> call to prep_temp_blob(). Such a change would also affect the "textconv"
> codepath and the result will start feeding smudged contents to the
> "textconv" filter, and I think the argument "external tools prefer to be
> fed smudged contents, not clean ones, because that is the representation
> tailored for the platform" would hold even more stronger in that context.
In practice, I don't think it will affect textconv users much. The point
of textconv is to munge ugly binary files into something humans can
read. I'm not sure if people are actually smudging those (though I guess
people have talked about smuding openoffice files, so anything is
possible...).
Anyway, I was planning to make a patch to always feed textconv the
_clean_ version of each file. My thinking was:
1. Then tools get a consistent view of the data across platforms.
I.e., my textconv munger or external diff script will work no
matter what you think the working tree should look like.
2. The tool may want the clean version, or it may want the smudged
version. Or it may be able to operate on either. If we give it a
format it doesn't like, it will have to undo whatever we did.
For most cases, we start with the clean file (i.e., from a tree or
from the index). If we hand out the clean file and the script
doesn't like it, it pays the cost to smudge once. If we hand it the
smudged file and the script doesn't like it, we pay the cost to
smudge _and_ the script pays the cost to clean.
But I have to admit that I have never once used a clean/smudge filter in
an actual project. So I am coming at this purely from a hypothetical
position. I do think the format that any scripts get should be
consistent (i.e., always clean or always smudged), and textconv and
extdiff should use the same format.
One thing that I have considered that may make it moot is adding a
"fast" extdiff/textconv interface: instead of writing temp files, call
the script with the quick information (like sha-1 and filename), and let
it ask git for the data as appropriate.
In my case, I'm motivated by making textconv faster for my media repo;
the textconv script just displays the exif (and similar) metadata for
the files. So right now for each diffed file it writes out the entire
large file to a tempfile, reads the exif data, and prints it. If it got
just the sha-1 it could:
- use its own caching layer (which my metadata reading tools already
have) to avoid looking at the file at all, since it knows the
textconv for a particular sha-1 is immutable
- when it _does_ have to read, it can stream only as much data as it
needs to get the metadata and never touch the disk at all. I.e., by
piping from "git cat-file".
Given such an interface, it would also be trivial to ask cat-file for
your data with or without smudging, as appropriate. Which is not that
different from what you suggested earlier in the thread with "git
filter", except that instead of smudging the existing tempfile, we skip
the part where we write out the uninteresting bit. :)
So it's a little more work on the part of the script-writer (who now has
to know about getting the data from git instead of just opening some
files), but it can potentially be a lot faster. And of course I would
retain the original interface both for historical reasons and because it
is simpler to use; this would be diff.*.textconv-quick or similar.
> diff.c | 69 ++-------------------------------------------------------------
> 1 files changed, 3 insertions(+), 66 deletions(-)
For some reason, with your patch the tempfiles are created with mode
0005 for me (whereas they are usually 0505), which makes open() in the
called script unhappy. Looking over the patch text, though, I have no
idea what change could be causing that.
-Peff
^ permalink raw reply
* Re: [PATCH] git-branch: display "was sha1" on branch deletion rather than just "sha1"
From: Jeff King @ 2009-03-22 6:13 UTC (permalink / raw)
To: Brandon Casey; +Cc: Junio C Hamano, git
In-Reply-To: <ee63ef30903211709s24f8294dq9ea52bcae72b755a@mail.gmail.com>
On Sat, Mar 21, 2009 at 07:09:17PM -0500, Brandon Casey wrote:
> Make it more pleasant to read about a branch deletion by adding "was".
> Jeff King suggested this, and I ignored it. He was right.
Heh. While I am tempted to dance in a circle shouting "I told you so", I
actually find that after getting used to it, I do not mind the current
output. ;)
Out of curiosity, what prompted your change of heart? Did you hear from
somebody who found it confusing, or did you just change your mind?
-Peff
^ permalink raw reply
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
From: Jeff King @ 2009-03-22 6:19 UTC (permalink / raw)
To: Ben Walton; +Cc: GIT List
In-Reply-To: <1237635198-sup-2111@ntdws12.chass.utoronto.ca>
On Sat, Mar 21, 2009 at 07:38:35AM -0400, Ben Walton wrote:
> I used the ifndef/endif setup becuase that's how the PERL_PATH was set
> and also becuase I think it's slightly more explicit. I'm ok with ?=
I can't think of any reason why the two would not be equivalent
functionally. I would generally use ?= because it is more portable, but
we are inextricably bound to gmake at this point, so I don't think that
matters. So I don't have a strong preference.
> > but maybe it is not worth caring about (since it may complicate building
> > Documentation if you _haven't_ build the actual code).
>
> In my case, I'm using the configure script and then running make,
> which sees the Documentation/Makefile source in the ../config.mak
> files, so there may be some variance between pure make and make +
> autoconf in this respect. I hadn't looked at it in that light.
> Should this be reconciled too?
Oh, right, I forgot that it pulls in config.mak. So it is really a
non-issue if you are putting SHELL_PATH in your config.mak (or defining
it via autoconf). So nevermind my ramblings in that direction.
I think it should be fine to just resend your patch with:
1. default to $(SHELL)
2. quote $(SHELL_PATH) as appropriate
-Peff
^ permalink raw reply
* Re: [PATCHv2 1/3] format-patch: create patch filename in one function
From: Junio C Hamano @ 2009-03-22 6:53 UTC (permalink / raw)
To: Stephen Boyd; +Cc: git
In-Reply-To: <49C5D3BC.3030100@gmail.com>
Stephen Boyd <bebarino@gmail.com> writes:
> Junio C Hamano wrote:
> ...
>> IOW, you can introduce a new format specifier (say, "%f") to
>> format_commit_message() and the implemention of get_patch_filename() would
>> just prepare a strbuf and call format_commit_message() on it, no?
>
> This sounds great! I'm new so I don't know where to look for something
> like this.
I suspect you may not even have to pass the generated string around if you
did so. Instead, you could pass the commit to log_write_email_headers()
instead of sha1_to_hex(commit->object.sha1) from show_log(), and use the
sha-1on the unix "From " line, and inside "if (opt->mime_boundar)", you
can ask format_commit_message("%f") to come up with a filename.
^ permalink raw reply
* Re: [PATCHv2 1/3] format-patch: create patch filename in one function
From: Stephen Boyd @ 2009-03-22 6:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7veiwq82wc.fsf@gitster.siamese.dyndns.org>
Junio C Hamano wrote:
> Stephen Boyd <bebarino@gmail.com> writes:
>
>> Junio C Hamano wrote:
>> ...
>>> IOW, you can introduce a new format specifier (say, "%f") to
>>> format_commit_message() and the implemention of get_patch_filename() would
>>> just prepare a strbuf and call format_commit_message() on it, no?
>> This sounds great! I'm new so I don't know where to look for something
>> like this.
>
> I suspect you may not even have to pass the generated string around if you
> did so. Instead, you could pass the commit to log_write_email_headers()
> instead of sha1_to_hex(commit->object.sha1) from show_log(), and use the
> sha-1on the unix "From " line, and inside "if (opt->mime_boundar)", you
> can ask format_commit_message("%f") to come up with a filename.
I believe I won't be able to get the patch suffix at that point in the
code. Unless I decide to add that to the rev_info instead?
^ permalink raw reply
* Re: [PATCH] Respect core.autocrlf when preparing temporary files for external diff
From: Junio C Hamano @ 2009-03-22 7:18 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git, Sebastian Schuberth
In-Reply-To: <20090322061046.GA14765@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ...how expensive is the check for convert_to_working_tree? It should
> just be a gitattributes lookup, shouldn't it? Which:
>
> a. we are doing anyway and caching
>
> b. which takes a fraction of a second (try "time git ls-files | git
> check-attr --stdin diff >/dev/null", which should give a
> worst-case).
Ok. Although I already queued the removal to 'pu' for tonight's pushout
and it is way too late to revert that, I think I didn't have to remove the
function. The codepath that lets you cheat by borrowing from the checkout
runs convert_to_git() when it borrows, and if you are seeing a meaningful
optimization even with that overhead, perhaps it would be worth keeping.
There is another check that should be there but is missing from the
current implementation of reuse_worktree_file(), other than the "is
convert_to_working_tree() a no-op for this path?" check. The last check
in the function would say "Yeah, we can reuse it" if the ce is marked
"assume unchanged"; we do not want to blindly reuse the file from the work
tree in that case.
> Anyway, I was planning to make a patch to always feed textconv the
> _clean_ version of each file. My thinking was:
>
> 1. Then tools get a consistent view of the data across platforms.
> I.e., my textconv munger or external diff script will work no
> matter what you think the working tree should look like.
>
> 2. The tool may want the clean version, or it may want the smudged
> version. Or it may be able to operate on either. If we give it a
> format it doesn't like, it will have to undo whatever we did.
>
> For most cases, we start with the clean file (i.e., from a tree or
> from the index). If we hand out the clean file and the script
> doesn't like it, it pays the cost to smudge once. If we hand it the
> smudged file and the script doesn't like it, we pay the cost to
> smudge _and_ the script pays the cost to clean.
While the purist in me says #1 above is the right argument to make for
feeding "clean" version, I suspect that the textconv or extdiff tools more
often are not made from scratch and ported across platforms than are
cobbled up together out of tools the script writer finds on his platform.
I suspect that Dscho's "a tempfile should look like a checkout" would be
much friendlier to them in practice for this reason.
> For some reason, with your patch the tempfiles are created with mode
> 0005 for me (whereas they are usually 0505), which makes open() in the
> called script unhappy. Looking over the patch text, though, I have no
> idea what change could be causing that.
Neither 0005 nor 0505 sounds correct to me; shouldn't they be 0600 or
something like that?
^ permalink raw reply
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