* [PATCH/RFC] clean: add a flag to back up cleaned files
@ 2014-05-27 14:17 Erik Faye-Lund
2014-05-27 16:37 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Erik Faye-Lund @ 2014-05-27 14:17 UTC (permalink / raw)
To: git
The combination of "git clean" and fat fingers can some times cause
data-loss, which can be frustrating.
So let's add a flag that imports the files to be deleted into the
object-database, in a way similar to what git-stash does. Maintain
a reflog of the previously backed up clean-runs.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
I've had a similar patch laying around for quite a while, but since
f538a91 ("git-clean: Display more accurate delete messages"), this
patch is a lot less nasty than before. So here you go, perhaps
someone else has fat fingers and hate to lose work?
Documentation/config.txt | 4 ++
Documentation/git-clean.txt | 4 ++
builtin/clean.c | 111 +++++++++++++++++++++++++++++++++++++++++++-
t/t7300-clean.sh | 12 +++++
4 files changed, 130 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..d58fe31 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -797,6 +797,10 @@ clean.requireForce::
A boolean to make git-clean do nothing unless given -f,
-i or -n. Defaults to true.
+clean.backup::
+ A boolean to make git-clean back up files before they are
+ deleted. Defaults to false.
+
color.branch::
A boolean to enable/disable color in the output of
linkgit:git-branch[1]. May be set to `always`,
diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
index 8997922..bc9d703 100644
--- a/Documentation/git-clean.txt
+++ b/Documentation/git-clean.txt
@@ -41,6 +41,10 @@ OPTIONS
--interactive::
Show what would be done and clean files interactively. See
``Interactive mode'' for details.
+-b::
+--backup::
+ Back up files to a reflog before deleting them. The tree of
+ backed up files are stored in the reflog for refs/clean-backup.
-n::
--dry-run::
diff --git a/builtin/clean.c b/builtin/clean.c
index 9a91515..96fb4b2 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -16,9 +16,12 @@
#include "column.h"
#include "color.h"
#include "pathspec.h"
+#include "tree-walk.h"
+#include "unpack-trees.h"
+#include "cache-tree.h"
static int force = -1; /* unset */
-static int interactive;
+static int interactive, backup;
static struct string_list del_list = STRING_LIST_INIT_DUP;
static unsigned int colopts;
@@ -120,6 +123,11 @@ static int git_clean_config(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp(var, "clean.backup")) {
+ backup = git_config_bool(var, value);
+ return 0;
+ }
+
if (!strcmp(var, "clean.requireforce")) {
force = !git_config_bool(var, value);
return 0;
@@ -148,6 +156,93 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
return 0;
}
+static int backed_up_anything;
+
+static void backup_file(const char *path, struct stat *st)
+{
+ if (add_to_cache(path, st, 0))
+ die(_("backing up '%s' failed"), path);
+ backed_up_anything = 1;
+}
+
+static struct commit_list *parents;
+
+static void prepare_backup(void)
+{
+ struct unpack_trees_options opts;
+ unsigned char sha1[20];
+ struct tree *tree;
+ struct commit *parent;
+ struct tree_desc t;
+
+ if (get_sha1("HEAD", sha1))
+ die(_("You do not have the initial commit yet"));
+
+ /* prepare parent-list */
+ parent = lookup_commit_or_die(sha1, "HEAD");
+ commit_list_insert(parent, &parents);
+
+ /* load HEAD into the index */
+
+ tree = parse_tree_indirect(sha1);
+ if (!tree)
+ die(_("Failed to unpack tree object %s"), sha1);
+
+ parse_tree(tree);
+ init_tree_desc(&t, tree->buffer, tree->size);
+
+ memset(&opts, 0, sizeof(opts));
+ opts.head_idx = -1;
+ opts.src_index = &the_index;
+ opts.dst_index = &the_index;
+ opts.index_only = 1;
+
+ if (unpack_trees(1, &t, &opts)) {
+ /* We've already reported the error, finish dying */
+ exit(128);
+ }
+}
+
+static void finish_backup(void)
+{
+ const char *ref = "refs/clean-backup";
+ unsigned char commit_sha1[20];
+ struct strbuf msg = STRBUF_INIT;
+ char logfile[PATH_MAX];
+ struct stat st;
+
+ if (!backed_up_anything)
+ return;
+
+ if (!active_cache_tree)
+ active_cache_tree = cache_tree();
+
+ if (!cache_tree_fully_valid(active_cache_tree)) {
+ if (cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const *)active_cache,
+ active_nr, 0) < 0)
+ die("failed to update cache");
+ }
+
+ strbuf_addstr(&msg, "Automatically committed by git-clean");
+
+ /* create a reflog, if there isn't one */
+ git_snpath(logfile, sizeof(logfile), "logs/%s", ref);
+ if (stat(logfile, &st)) {
+ FILE *fp = fopen(logfile, "w");
+ if (!fp)
+ warning(_("Can not do reflog for '%s'\n"), ref);
+ else
+ fclose(fp);
+ }
+
+ if (commit_tree(&msg, active_cache_tree->sha1, parents, commit_sha1,
+ NULL, NULL))
+ die("failed to commit :(");
+
+ update_ref(msg.buf, ref, commit_sha1, NULL, 0, DIE_ON_ERR);
+}
+
static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
int dry_run, int quiet, int *dir_gone)
{
@@ -207,6 +302,8 @@ static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
*dir_gone = 0;
continue;
} else {
+ if (backup && !dry_run)
+ backup_file(path->buf, &st);
res = dry_run ? 0 : unlink(path->buf);
if (!res) {
quote_path_relative(path->buf, prefix, "ed);
@@ -878,6 +975,8 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
OPT_BOOL('i', "interactive", &interactive, N_("interactive cleaning")),
OPT_BOOL('d', NULL, &remove_directories,
N_("remove whole directories")),
+ OPT_BOOL('b', "backup", &backup,
+ N_("back up files to a reflog before deleting them")),
{ OPTION_CALLBACK, 'e', "exclude", &exclude_list, N_("pattern"),
N_("add <pattern> to ignore rules"), PARSE_OPT_NONEG, exclude_cb },
OPT_BOOL('x', NULL, &ignored, N_("remove ignored files, too")),
@@ -922,6 +1021,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
if (!ignored)
setup_standard_excludes(&dir);
+ if (backup && !dry_run)
+ prepare_backup();
+
el = add_exclude_list(&dir, EXC_CMDL, "--exclude option");
for (i = 0; i < exclude_list.nr; i++)
add_exclude(exclude_list.items[i].string, "", 0, el, -(i+1));
@@ -985,6 +1087,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
printf(dry_run ? _(msg_would_remove) : _(msg_remove), qname);
}
} else {
+ if (backup && !dry_run)
+ backup_file(abs_path.buf, &st);
+
res = dry_run ? 0 : unlink(abs_path.buf);
if (res) {
qname = quote_path_relative(item->string, NULL, &buf);
@@ -1002,5 +1107,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
strbuf_release(&buf);
string_list_clear(&del_list, 0);
string_list_clear(&exclude_list, 0);
+
+ if (backup && !dry_run)
+ finish_backup();
+
return (errors != 0);
}
diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 74de814..27d1d74 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -527,4 +527,16 @@ test_expect_success 'git clean -d respects pathspecs (pathspec is prefix of dir)
test_path_is_dir foobar
'
+test_expect_success 'git clean -b' '
+ git reset --hard HEAD &&
+ git clean -dfx &&
+ mkdir -p foobar &&
+ echo "bar" > bar &&
+ echo "baz" > foobar/baz &&
+ git clean -d -f -b &&
+ git diff --name-only refs/clean-backup@{0}^ refs/clean-backup@{0} >actual &&
+ grep bar actual &&
+ grep foobar/baz actual
+'
+
test_done
--
1.9.2.msysgit.0.161.g83227c1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH/RFC] clean: add a flag to back up cleaned files
2014-05-27 14:17 [PATCH/RFC] clean: add a flag to back up cleaned files Erik Faye-Lund
@ 2014-05-27 16:37 ` Jeff King
2014-05-27 18:12 ` Erik Faye-Lund
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2014-05-27 16:37 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git
On Tue, May 27, 2014 at 04:17:34PM +0200, Erik Faye-Lund wrote:
> The combination of "git clean" and fat fingers can some times cause
> data-loss, which can be frustrating.
>
> So let's add a flag that imports the files to be deleted into the
> object-database, in a way similar to what git-stash does. Maintain
> a reflog of the previously backed up clean-runs.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
> I've had a similar patch laying around for quite a while, but since
> f538a91 ("git-clean: Display more accurate delete messages"), this
> patch is a lot less nasty than before. So here you go, perhaps
> someone else has fat fingers and hate to lose work?
I've definitely considered doing something like this before (and for
"git reset --hard"). My biggest concern would be poor performance in
some cases. But since it's optional, and one can presumably override it
with --no-backup for a specific large cleanup, it would not hurt anybody
who does not want to play with it.
> + /* load HEAD into the index */
> +
> + tree = parse_tree_indirect(sha1);
> + if (!tree)
> + die(_("Failed to unpack tree object %s"), sha1);
> +
> + parse_tree(tree);
> + init_tree_desc(&t, tree->buffer, tree->size);
> +
> + memset(&opts, 0, sizeof(opts));
> + opts.head_idx = -1;
> + opts.src_index = &the_index;
> + opts.dst_index = &the_index;
> + opts.index_only = 1;
> +
> + if (unpack_trees(1, &t, &opts)) {
> + /* We've already reported the error, finish dying */
> + exit(128);
> + }
This bit of the code surprised me. I guess you are trying to re-create
the index state of the HEAD so that the commit you build on top of it
contains _only_ the untracked files as changes, and not whatever
intermediate index state you had. That makes some sense to me, as clean
is never touching the index state.
Though taking a step back for a moment, I'd like to think about doing
something similar for "reset --hard", which is the other "destructive"
operation in git[1]. In that case, I think saving the index state is also
useful, because it is being reset, too (and while those blobs are
recoverable, the exact state is sometimes useful).
If we were to do that, should it be a separate ref? Or should there be a
single backup ref for such "oops, undo that" operations? If the latter,
what should that ref look like? I think it would look something like
refs/stash, with the index and the working tree state stored as separate
commits (even though in the "clean" case, the index state is not likely
to be that interesting, it is cheap to store and makes the recovery
tools uniform to use).
And if we are going to store it like that, should we just be using "git
stash save --keep-index --include-untracked"? I think we would just need
to teach it a "--no-reset" option to leave the tracked files as-is.
I realize that I went a few steps down the "if..." chain there to get to
"should we just be using stash?". But it would also make the code here
dirt-simple.
[1] Actually, "reset --hard" may be more of an education issue, as
simply running "git stash" has the same effect, but keeps a backup.
It's just that "reset --hard" is advertised as the solution to many
problems.
> + if (!active_cache_tree)
> + active_cache_tree = cache_tree();
> +
> + if (!cache_tree_fully_valid(active_cache_tree)) {
> + if (cache_tree_update(active_cache_tree,
> + (const struct cache_entry * const *)active_cache,
> + active_nr, 0) < 0)
> + die("failed to update cache");
> + }
I'd have thought you could use write_cache_as_tree, which backs "git
write-tree", but there is currently no way to convince it not to write
out the new cache. This is little enough code that it may not be worth
refactoring write_cache_as_tree to handle it.
> + /* create a reflog, if there isn't one */
> + git_snpath(logfile, sizeof(logfile), "logs/%s", ref);
> + if (stat(logfile, &st)) {
> + FILE *fp = fopen(logfile, "w");
> + if (!fp)
> + warning(_("Can not do reflog for '%s'\n"), ref);
> + else
> + fclose(fp);
> + }
Kind of gross that we have to do this ourselves (and somewhat contrary
to efforts elsewhere to make the ref code more abstract), but I see that
"git stash" does the same thing.
Should you fopen with "a" here, to avoid a race condition where another
process creates it between the stat() and the fopen(), in which case we
would truncate what they wrote? You could even just get rid of the
stat(), then, like:
if ((fp = fopen(logfile, "a")))
fclose(fp);
else
warning(_("Can not do reflog for '%s'"), ref);
Also note that your warning has an extra "\n" in it.
-Peff
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH/RFC] clean: add a flag to back up cleaned files
2014-05-27 16:37 ` Jeff King
@ 2014-05-27 18:12 ` Erik Faye-Lund
2014-05-27 18:48 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Erik Faye-Lund @ 2014-05-27 18:12 UTC (permalink / raw)
To: Jeff King; +Cc: GIT Mailing-list
On Tue, May 27, 2014 at 6:37 PM, Jeff King <peff@peff.net> wrote:
> On Tue, May 27, 2014 at 04:17:34PM +0200, Erik Faye-Lund wrote:
>
>> The combination of "git clean" and fat fingers can some times cause
>> data-loss, which can be frustrating.
>>
>> So let's add a flag that imports the files to be deleted into the
>> object-database, in a way similar to what git-stash does. Maintain
>> a reflog of the previously backed up clean-runs.
>>
>> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
>> ---
>> I've had a similar patch laying around for quite a while, but since
>> f538a91 ("git-clean: Display more accurate delete messages"), this
>> patch is a lot less nasty than before. So here you go, perhaps
>> someone else has fat fingers and hate to lose work?
>
> I've definitely considered doing something like this before (and for
> "git reset --hard"). My biggest concern would be poor performance in
> some cases. But since it's optional, and one can presumably override it
> with --no-backup for a specific large cleanup, it would not hurt anybody
> who does not want to play with it.
I also made it opt-in rather than opt-out by default. Perhaps it
shouldn't be, though - dunno.
>> + /* load HEAD into the index */
>> +
>> + tree = parse_tree_indirect(sha1);
>> + if (!tree)
>> + die(_("Failed to unpack tree object %s"), sha1);
>> +
>> + parse_tree(tree);
>> + init_tree_desc(&t, tree->buffer, tree->size);
>> +
>> + memset(&opts, 0, sizeof(opts));
>> + opts.head_idx = -1;
>> + opts.src_index = &the_index;
>> + opts.dst_index = &the_index;
>> + opts.index_only = 1;
>> +
>> + if (unpack_trees(1, &t, &opts)) {
>> + /* We've already reported the error, finish dying */
>> + exit(128);
>> + }
>
> This bit of the code surprised me. I guess you are trying to re-create
> the index state of the HEAD so that the commit you build on top of it
> contains _only_ the untracked files as changes, and not whatever
> intermediate index state you had. That makes some sense to me, as clean
> is never touching the index state.
TBH, I didn't really think this stuff through, I basically just hacked
on this until I got it to save away superfluous files when the index
matched HEAD. So this part is more accidental than designed. I'm not
very familiar with the index-maniuplation code in Git either.
I *think* the right thing to do would be to save the tree of HEAD
*plus* those deleted files in this case. That way it only records the
destruction itself. This does *not* seem to be what currently happens.
If I have a change staged, that staged change also gets included in
the commit. That's not ideal, I think.
> Though taking a step back for a moment, I'd like to think about doing
> something similar for "reset --hard", which is the other "destructive"
> operation in git[1]. In that case, I think saving the index state is also
> useful, because it is being reset, too (and while those blobs are
> recoverable, the exact state is sometimes useful).
I agree. I guess that should essentially do a "full" git-stash.
> If we were to do that, should it be a separate ref? Or should there be a
> single backup ref for such "oops, undo that" operations? If the latter,
> what should that ref look like? I think it would look something like
> refs/stash, with the index and the working tree state stored as separate
> commits (even though in the "clean" case, the index state is not likely
> to be that interesting, it is cheap to store and makes the recovery
> tools uniform to use).
Hmm, perhaps. I do like the concept of a "git undo" of sorts, but
perhaps that'll raise the expectations even further? Or maybe raising
them a good thing, so people add missing features? :)
> And if we are going to store it like that, should we just be using "git
> stash save --keep-index --include-untracked"? I think we would just need
> to teach it a "--no-reset" option to leave the tracked files as-is.
Hm, interesting. But it does leave me with a bit of a bad feeling; git
stash is currently a shell-script, and I want us to move *away* from
depending on those rather than towards... Or perhaps I just convinced
myself to port git-stash to C? I guess the full script won't be
needed, only the heavy lifting...
> I realize that I went a few steps down the "if..." chain there to get to
> "should we just be using stash?". But it would also make the code here
> dirt-simple.
Simplicity is good, and it's always good to hear some different ideas
on how to reach a goal.
> [1] Actually, "reset --hard" may be more of an education issue, as
> simply running "git stash" has the same effect, but keeps a backup.
> It's just that "reset --hard" is advertised as the solution to many
> problems.
>
>> + if (!active_cache_tree)
>> + active_cache_tree = cache_tree();
>> +
>> + if (!cache_tree_fully_valid(active_cache_tree)) {
>> + if (cache_tree_update(active_cache_tree,
>> + (const struct cache_entry * const *)active_cache,
>> + active_nr, 0) < 0)
>> + die("failed to update cache");
>> + }
>
> I'd have thought you could use write_cache_as_tree, which backs "git
> write-tree", but there is currently no way to convince it not to write
> out the new cache. This is little enough code that it may not be worth
> refactoring write_cache_as_tree to handle it.
>
I think not having to maintain multiple copies might make it worth
factoring it out. Sure, it's not *that* complicated, but it *is*
pretty well-contained.
>> + /* create a reflog, if there isn't one */
>> + git_snpath(logfile, sizeof(logfile), "logs/%s", ref);
>> + if (stat(logfile, &st)) {
>> + FILE *fp = fopen(logfile, "w");
>> + if (!fp)
>> + warning(_("Can not do reflog for '%s'\n"), ref);
>> + else
>> + fclose(fp);
>> + }
>
> Kind of gross that we have to do this ourselves (and somewhat contrary
> to efforts elsewhere to make the ref code more abstract), but I see that
> "git stash" does the same thing.
Hmm, it seems like log_ref_setup might be able to do the work for us,
if it wold take a flag to force O_CREAT. Perhaps that's worth the
effort?
> Should you fopen with "a" here, to avoid a race condition where another
> process creates it between the stat() and the fopen(), in which case we
> would truncate what they wrote? You could even just get rid of the
> stat(), then, like:
>
> if ((fp = fopen(logfile, "a")))
> fclose(fp);
> else
> warning(_("Can not do reflog for '%s'"), ref);
>
> Also note that your warning has an extra "\n" in it.
Good catch, thanks. Both details :)
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH/RFC] clean: add a flag to back up cleaned files
2014-05-27 18:12 ` Erik Faye-Lund
@ 2014-05-27 18:48 ` Jeff King
0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2014-05-27 18:48 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: GIT Mailing-list
On Tue, May 27, 2014 at 08:12:52PM +0200, Erik Faye-Lund wrote:
> > I've definitely considered doing something like this before (and for
> > "git reset --hard"). My biggest concern would be poor performance in
> > some cases. But since it's optional, and one can presumably override it
> > with --no-backup for a specific large cleanup, it would not hurt anybody
> > who does not want to play with it.
>
> I also made it opt[...]in rather than opt[...]out by default. Perhaps it
> shouldn't be, though - dunno.
I like starting with it optional, and then people who are interested in
the feature can experiment with it, giving it a chance to prove itself
(and for us to work out any downsides) before turning it on by default.
BTW, I think the opt phrases above might be caught by vger's taboo
filter. If your mail doesn't appear on the list, that is likely the
reason.
> > This bit of the code surprised me. I guess you are trying to re-create
> > the index state of the HEAD so that the commit you build on top of it
> > contains _only_ the untracked files as changes, and not whatever
> > intermediate index state you had. That makes some sense to me, as clean
> > is never touching the index state.
>
> TBH, I didn't really think this stuff through, I basically just hacked
> on this until I got it to save away superfluous files when the index
> matched HEAD. So this part is more accidental than designed. I'm not
> very familiar with the index-maniuplation code in Git either.
>
> I *think* the right thing to do would be to save the tree of HEAD
> *plus* those deleted files in this case. That way it only records the
> destruction itself. This does *not* seem to be what currently happens.
> If I have a change staged, that staged change also gets included in
> the commit. That's not ideal, I think.
Ah. Yeah, if you are going to just record the current index state, I do
not see a reason to call unpack_trees at all. You can just add new
entries to the index, and then throw the resulting index away without
writing it back to disk.
But I do think it would make sense to reset it back to HEAD and build
straight on there, in which case you basically want to do "read-tree
HEAD". Or in C code, unpack-trees as a "oneway merge". I thought that's
what was going on here. ;)
All of this is moot if you go in the stash direction, as then you would
be storing something a bit more complicated (and delegating the
complexity to stash anyway).
> > If we were to do that, should it be a separate ref? Or should there be a
> > single backup ref for such "oops, undo that" operations? If the latter,
> > what should that ref look like? I think it would look something like
> > refs/stash, with the index and the working tree state stored as separate
> > commits (even though in the "clean" case, the index state is not likely
> > to be that interesting, it is cheap to store and makes the recovery
> > tools uniform to use).
>
> Hmm, perhaps. I do like the concept of a "git undo" of sorts, but
> perhaps that'll raise the expectations even further? Or maybe raising
> them a good thing, so people add missing features? :)
Yeah, I think this would just be a first step on the way to "git undo".
It's the safety net for a few commands, but a true undo would need
somebody to build logic to see what the last command is, and then
reverse it (presumably using these safety nets). I don't think there's
any problem with building this first step and leaving the rest for
somebody to do later; it's a strict improvement.
> > And if we are going to store it like that, should we just be using "git
> > stash save --keep-index --include-untracked"? I think we would just need
> > to teach it a "--no-reset" option to leave the tracked files as-is.
>
> Hm, interesting. But it does leave me with a bit of a bad feeling; git
> stash is currently a shell-script, and I want us to move *away* from
> depending on those rather than towards... Or perhaps I just convinced
> myself to port git-stash to C? I guess the full script won't be
> needed, only the heavy lifting...
Yeah, I wondered if you might say that, knowing how you Windows guys are
hesitant about shell scripts. :)
My feeling is that you should think about the best design, and implement
it that way. If stash as a shell script is a problem, then fix that on
the way. Of course it is very easy for me to tell you that, as I am not
the one volunteering to do the extra work. ;)
> >> + if (!active_cache_tree)
> >> + active_cache_tree = cache_tree();
> >> +
> >> + if (!cache_tree_fully_valid(active_cache_tree)) {
> >> + if (cache_tree_update(active_cache_tree,
> >> + (const struct cache_entry * const *)active_cache,
> >> + active_nr, 0) < 0)
> >> + die("failed to update cache");
> >> + }
> >
> > I'd have thought you could use write_cache_as_tree, which backs "git
> > write-tree", but there is currently no way to convince it not to write
> > out the new cache. This is little enough code that it may not be worth
> > refactoring write_cache_as_tree to handle it.
> >
> I think not having to maintain multiple copies might make it worth
> factoring it out. Sure, it's not *that* complicated, but it *is*
> pretty well-contained.
Yeah, I was on the fence when I said that. I think it would depend on
what the refactor looks like.
> >> + /* create a reflog, if there isn't one */
> >> + git_snpath(logfile, sizeof(logfile), "logs/%s", ref);
> >> + if (stat(logfile, &st)) {
> >> + FILE *fp = fopen(logfile, "w");
> >> + if (!fp)
> >> + warning(_("Can not do reflog for '%s'\n"), ref);
> >> + else
> >> + fclose(fp);
> >> + }
> >
> > Kind of gross that we have to do this ourselves (and somewhat contrary
> > to efforts elsewhere to make the ref code more abstract), but I see that
> > "git stash" does the same thing.
>
> Hmm, it seems like log_ref_setup might be able to do the work for us,
> if it wold take a flag to force O_CREAT. Perhaps that's worth the
> effort?
log_ref_setup gets called internally by update_ref. I guess you'd have
to teach update_ref a new flag for "always create reflog", which would
not be too bad. I suspect that would also make the "new_branch_log" code
in checkout.c cleaner, too.
Alternatively, the logic for "should we create a reflog" is in
log_ref_setup. It knows about core.logAllRefUpdates, and does some magic
for various prefixes and HEAD. That logic could be taught about
"refs/stash" and any other similar refs we add.
-Peff
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-05-27 18:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-27 14:17 [PATCH/RFC] clean: add a flag to back up cleaned files Erik Faye-Lund
2014-05-27 16:37 ` Jeff King
2014-05-27 18:12 ` Erik Faye-Lund
2014-05-27 18:48 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).