git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Edward Thomson <ethomson@edwardthomson.com>
Cc: git@vger.kernel.org
Subject: Re: [RFC PATCH 1/1] recover: restoration of deleted worktree files
Date: Sat, 04 Aug 2018 08:54:49 -0700	[thread overview]
Message-ID: <xmqqpnyyt9di.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20180804142416.GA6@5f28dc333bbd> (Edward Thomson's message of "Sat, 4 Aug 2018 14:24:16 +0000")

Edward Thomson <ethomson@edwardthomson.com> writes:

> Introduce git-recover, a simple script to aide in restoration of deleted
> worktree files.  This will look for unreachable blobs in the object
> database and prompt users to restore them to disk, either interactively
> or on the command-line.
> ---
>  git-recover.sh | 311 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 311 insertions(+)
>  create mode 100755 git-recover.sh

My first reaction was to say that I am not going to take a new
command written only for bash with full bashism, even if it came
with docs, tests nor Makefile integration, for Git itself.  Then I
reconsidered, as not everything related to Git is git-core, and all
of the above traits are sign of this patch _not_ meant for git-core.

In other words, I think this patch can be a fine addition to
somebody else's project (i.e. random collection of scripts that may
help Git users), so let's see how I can offer comments/inputs to
help you improve it.  So I won't comment on lang, log message, or
shell scripting style---these are project convention and the git-core
convention won't be relevant to this patch.

> diff --git a/git-recover.sh b/git-recover.sh
> new file mode 100755
> index 000000000..651d4116f
> --- /dev/null
> +++ b/git-recover.sh
> @@ -0,0 +1,311 @@
> +#!/usr/bin/env bash
> +#
> +# This program helps recover files in your repository that were deleted
> +# from the working tree.
> +#
> +# Copyright (c) 2017-2018 Edward Thomson.
> +
> +set -e
> +
> +IFS=$'\n'
> +
> +PROGNAME=$(echo "$0" | sed -e 's/.*\///')
> +GIT_DIR=$(git rev-parse --git-dir)
> +
> +DO_RECOVER=0
> +DO_FULL=0
> +DO_INTERACTIVE=0
> +BLOBS=()
> +FILENAMES=()
> +
> +function die_usage {
> +	echo "usage: $PROGNAME [-a] [-i] [--full] [<id> [-f <filename>] ...]" >&2
> +	exit 1
> +}
> +
> +while [[ $# -gt 0 ]]; do
> +	case "$1" in
> +	-a|--all)
> +		DO_RECOVER=1
> +		;;
> +	-i|--interactive)
> +		DO_INTERACTIVE=1
> +		;;
> +	--full)
> +		DO_FULL=1
> +		;;
> +	*)
> +		if [ "${1:0:1}" == "-" ]; then
> +			echo "$PROGNAME: unknown argument: $1" >&2
> +			die_usage
> +		fi
> +		BLOBS+=("$1")
> +
> +		shift
> +		if [ "$1" == "-f" ] || [ "$1" == "--filename" ]; then
> +			shift
> +			if [ $# == 0 ]; then
> +				die_usage
> +			fi
> +			FILENAMES+=("$1")
> +			shift
> +		else
> +			FILENAMES+=("")
> +		fi

You do not want to take "--file=Makefile" (i.e. abbreviated option
name, and value as part of the option arg after '=')?

> +		continue
> +	;;
> +	esac
> +	shift
> +done

So, as a user, I can run this with "-a" but no blob object names to
run it in DO_RECOVER mode, or I can give one or more "blob spec"
where I say object id, optionally followed by one "-f filename"; in
the latter mode, BLOBS[] and FILENAMES[] array would have the same
number of elements, corresponding to each other.

> +if [ ${#BLOBS[@]} != 0 ] && [ $DO_RECOVER == 1 ]; then
> +	die_usage
> +elif [ ${#BLOBS[@]} != 0 ]; then
> +	DO_RECOVER=1
> +fi

If I did not say "-a" but did not give "blob spec", then I am
implicitly asking for "-a" to work in DO_RECOVER mode.

I think I understood what the program wants to do so far.

> +case "$OSTYPE" in
> +	darwin*|freebsd*) IS_BSD=1 ;;
> +	*) IS_BSD=0 ;;
> +esac
> +
> +function expand_given_blobs() {
> +	for i in "${!BLOBS[@]}"; do
> +		ID=$(git rev-parse --verify "${BLOBS[$i]}" 2>/dev/null || true)
> +
> +		if [ -z "$ID" ]; then
> +			echo "$PROGNAME: ${BLOBS[$i]} is not a valid object." 1>&2
> +			exit 1
> +		fi
> +
> +		TYPE=$(git cat-file -t "${ID}" 2>/dev/null || true)

An earlier "set -e" makes "|| true" ugliness required.  I suspect
use of "set -e" overall is a loss (vs explicit error checking).

> +		if [ "$TYPE" != "blob" ]; then
> +			echo "$PROGNAME: ${BLOBS[$i]} is not a blob." 1>&2
> +			exit
> +		fi

A user may have given us 11f5bcd9 and this function makes sure such
an object exists in the object store *and* is a blob.  Otherwise
it dies.  The main objective of this function is to turn that user
supplied object name to a full hex that is known to refer to an
existing blob.

> +		BLOBS[$i]=$ID
> +	done

I find a disconnect between this being a loop and the attiude "we
won't tolerate any erroneous input".  If a user is feeding dozens of
blob object names, wouldn't it be more helpful to give a warning, go
on and help the user with the rest?

> +}
> +
> +# find all the unreachable blobs
> +function find_unreachable() {
> +	FULLNESS="--no-full"
> +
> +	if [ $DO_FULL == 1 ]; then FULLNESS="--full"; fi
> +
> +	BLOBS=($(git fsck --unreachable --no-reflogs \
> +		"${FULLNESS}" --no-progress | sed -ne 's/^unreachable blob //p'))
> +}

If you are going to do a full sweep with fsck anyway, perhaps have
make it do the work of writing out lost-found, so that you can
iterate over them?

As a blob that is only reachable from a commit in reflog and not in
the histories that are alive is something a user would want to recover,
the use of --no-reflogs option makes sense to me.

> +function read_one_file {
> +	BLOB=$1
> +	FILTER_NAME=$2
> +	ARGS=()
> +
> +	if [ -z "$FILTER_NAME" ]; then
> +		ARGS+=("blob")
> +	else
> +		ARGS+=("--filters" "--path=$FILTER_NAME")
> +	fi
> +
> +	git cat-file "${ARGS[@]}" "$BLOB"
> +}

We get a blob object name and optional "--filename=name"; drives
cat-file possibly with "--filters --path=name" to grab its contents.
I find it a good thinking to use "--filters" that does the equivalent
of the smudge codepath, as you eventually want to materialize the
found contents as a working tree file ...

> +function write_one_file {
> +	BLOB=$1
> +	FILTER_NAME=$2
> +	OUTPUT_NAME=$3
> +
> +	ABBREV=$(git rev-parse --short "${BLOB}")
> +
> +	echo -n "Writing $ABBREV: "
> +	read_one_file "$BLOB" "$FILTER_NAME" > "$OUTPUT_NAME"
> +	echo "$OUTPUT_NAME."
> +}

... which happens here.

> +function unique_filename {
> +	if [ ! -f "${BLOB}" ]; then
> +		echo "$BLOB"
> +	else
> +		cnt=1
> +		while true
> +		do
> +			fn="${BLOB}~${cnt}"
> +			if [ ! -f "${fn}" ]; then
> +				echo "${fn}"
> +				break
> +			fi
> +			cnt=$((cnt+1))
> +		done
> +	fi
> +}

The function comes up with a name for a given blob to be written in
the directory, in which the user happened to have started the
command.  The function cannot be used unless we are processing each
blob fully before moving onto the next one. In other words,
"resurrect abcde --file=Makefile abcde --file=README.pdf" would
leave the same blob in the BLOBS[] array twice, so that two
different --filters can be attempted, but the calling code cannot
first call this function for all the BLOBS[] elements to assign them
unique-filename, as this function depends on "test -f" to be able to
see what happened to the previous elements in the BLOBS[] array.

> +function write_recoverable {
> +	for i in "${!BLOBS[@]}"; do
> +		BLOB=${BLOBS[$i]}
> +		FILTER_NAME=${FILENAMES[$i]}
> +		OUTPUT_NAME=${FILENAMES[$i]:-$(unique_filename)}
> +
> +		write_one_file "$BLOB" "$FILTER_NAME" "$OUTPUT_NAME"
> +	done
> +}

And we do that for all in BLOBS[].

> +function interactive {
> +	echo "Recoverable orphaned git blobs:"
> +
> +	sort_by_timestamp
> +	for BLOB_AND_TIMESTAMP in "${BLOB_AND_TIMESTAMPS[@]}"; do
> +		echo
> +
> +		BLOB=${BLOB_AND_TIMESTAMP::40}
> +		TIME=${BLOB_AND_TIMESTAMP:41}
> +		DATE=$([ ! -z "$TIME" ] && timestamp_to_s "$TIME" || echo "(Unknown)") 
> +
> +		echo "$BLOB  ($DATE)"
> +		show_summary "${BLOB}" | head -4 | sed -e 's/^/> /'
> +		echo
> +
> +		while true
> +		do
> +			echo -n "Recover this file? [y,n,v,f,q,?]: "
> +			read -r ans || return 1
> +
> +			case "$ans" in
> +			[yY]*)
> +				write_one_file "${BLOB}" "" "$(unique_filename)"
> +				break
> +				;;
> +			[nN]*)
> +				break
> +				;;
> +			[vV]*)
> +				view_file "${BLOB}"
> +				echo
> +				;;
> +			[fF]*)
> +				prompt_for_filename
> +
> +				if [ "$FILENAME" == "" ]; then
> +					break
> +				fi
> +
> +				write_one_file "${BLOB}" "${FILENAME}" "${FILENAME}"
> +				break
> +				;;
> +			\?*)
> +				echo
> +				echo "Do you want to recover this file?"
> +				echo " y: yes, write the file to ${BLOB}"
> +				echo " n: no, skip this file and see the next orphaned file"
> +				echo " v: view the file"
> +				echo " f: prompt for a filename to use for recovery"
> +				echo " q: quit"
> +				echo
> +				;;
> +			[qQ]*)
> +				return 0
> +				;;
> +			esac
> +		done
> +	done

Shows a bit of snippet from an orphaned blob, offer to write to
disk, to show it in full to give larger clue on its contents, etc.
I am not sure the value of [yY] that does not do the prompt thing,
possibly using the generated uniq name as the default---after all,
this is interactive.

One thing that I am mildly disappointed about this script is that it
does not give much over lost-found service fsck gives, other than
the "interactive" loop.  A dedicated "resurrect" command should do a
lot more.

Let me throw one piece of idea out, which may or may not work well.

"fsck" finds unreachable blobs, but it should be finding trees that
are also unreachable that contain them, recursively.  A command to
truly help users recover lost blobs should be taking advantage of
that fact and spending cycles to exploit it to help them.  For
example, you could (and this won't happen inside a bash script)

 - enumerate unreachable trees and blobs;
 - identify _a_ tree the sought-blob appears in;
 - identify _a_ tree that tree appears in;
 - recursively do the above until you find no more.

That gives you _a_ path to the blob in _a_ tree.  That top tree
might be the toplevel of a working tree (it may be referenced by a
commit that is unreachable, for example), or the commit and its top
level tree may already have been lost to gc and what you have might
be a tree structure representing a subtree (e.g. you thought that
you found "Makefile", but it turns out not to be the top-level
Makefile, but "t/Makefile").

But any such name is 1000x better than a random name derived from
the object name.

  reply	other threads:[~2018-08-04 15:58 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-04 14:22 [RFC PATCH 0/1] Introduce git-recover Edward Thomson
2018-08-04 14:24 ` [RFC PATCH 1/1] recover: restoration of deleted worktree files Edward Thomson
2018-08-04 15:54   ` Junio C Hamano [this message]
2018-08-04 16:17     ` Robert P. J. Day
2018-08-04 17:33       ` Todd Zullinger
2018-08-04 16:19     ` Edward Thomson
2018-08-04 16:48       ` Junio C Hamano
2018-08-05  1:34 ` [RFC PATCH 0/1] Introduce git-recover Jonathan Nieder

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqpnyyt9di.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=ethomson@edwardthomson.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).