Re: [PATCH 04/16] update-index: generalize 'read_index_info'
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 11, 2024, 22:45 UTC
- Message-ID
- <xmqqa5jrt7x4.fsf@gitster.g>
- In-Reply-To
- <9d0689e9c285b375b0067760929011038c085d65.1718130288.git.gitgitgadget@gmail.com>
"Victoria Dye via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 13 quoted lines
> From: Victoria Dye <vdye@github.com> > > Move 'read_index_info()' into a new header 'index-info.h' and generalize the > function to call a provided callback for each parsed line. Update > 'update-index.c' to use this generalized 'read_index_info()', adding the > callback 'apply_index_info()' to verify the parsed line and update the index > according to its contents. > > The input parsing done by 'read_index_info()' is similar to, but more > flexible than, the parsing done in 'mktree' by 'mktree_line()' (handling not > only 'git ls-tree' output but also the outputs of 'git apply --index-info' > and 'git ls-files --stage' outputs). To make 'mktree' more flexible, a later > patch will replace mktree's custom parsing with 'read_index_info()'.
"git apply --index-info"?
That is a blast from the past. It no longer exists since 7a988699 (apply: get rid of --index-info in favor of --build-fake-ancestor, 2007-09-17).
As to the scriptability, supporting "ls-files -s" and "ls-tree -r" output as our input do help, but the third one is not natively emitted and it is very unlikely that there are third-party tools that give output in that format. After all these years, I suspect that it is sufficient to say
"update-index --index-info" and "mktree" both read information
necessary to eventually build trees, but having two separate
parsers is a maintenance burden, so we are massaging the code
from the former to be reusable.without mentioning where the old third format comes from.
Show 28 quoted lines
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index d343416ae26..77df380cb54 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -11,6 +11,7 @@
> #include "gettext.h"
> #include "hash.h"
> #include "hex.h"
> +#include "index-info.h"
> #include "lockfile.h"
> #include "quote.h"
> #include "cache-tree.h"
> @@ -509,100 +510,29 @@ static void update_one(const char *path)
> report("add '%s'", path);
> }
>
> +static int apply_index_info(unsigned int mode, struct object_id *oid, int stage,
> + const char *path_name, void *cbdata UNUSED)
> {
> + if (!verify_path(path_name, mode)) {
> + fprintf(stderr, "Ignoring path %s\n", path_name);
> + return 0;
> + }
>
> + if (!mode) {
> + /* mode == 0 means there is no such path -- remove */
> + if (remove_file_from_index(the_repository->index, path_name))
> + die("git update-index: unable to remove %s", path_name);This changes the error message. We used to feed "ptr" (no longer visible to this function, as the caller unquotes before calling us) that pointed at the original the user gave to the program; now we report the path_name which is the result of the unquoting.
Show 8 quoted lines
> + }
> + else {
> + /* mode ' ' sha1 '\t' name
> + * ptr[-1] points at tab,
> + * ptr[-41] is at the beginning of sha1
> */
> + if (add_cacheinfo(mode, oid, path_name, stage))
> + die("git update-index: unable to update %s", path_name);But this side used to report the path_name as the result of unquoting in the original. So the above change would probably be OK in the name of consistency?
973d6a20 (update-index --index-info: adjust for funny-path quoting., 2005-10-16) was the origin of the unquoting, and looking at that commit, I have a feeling that the "ptr" thing above (i.e., the one I pointed out as changing the behaviour) was simply forgotten (as opposed to deliberately made to report the original) while updating the code to deal with quoted original into unquoted paths.
So I think the change is more than OK. It is a very welcome (belated) bugfix for 973d6a20 ;-).
> } > + > + return 0; > }
It looks a bit disappointing that we die in the callback like above, when the main parser loop that moved to the other file to be more reusable is now capable of returning to the caller with an error, but at this step, it is a good place to stop. A refactor that does not change the behaviour.
Nicely done.
Show 13 quoted lines
> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh > index cc72ead79f3..29696ade0d0 100755 > --- a/t/t2107-update-index-basic.sh > +++ b/t/t2107-update-index-basic.sh > @@ -142,4 +142,31 @@ test_expect_success '--index-version' ' > test_must_be_empty actual > ' > > +test_expect_success '--index-info fails on malformed input' ' > + # empty line > + echo "" | > + test_must_fail git update-index --index-info 2>err && > + grep "malformed input line" err &&
Using "test_grep" would make it easier to diagnose when test breaks. A failing "grep" will be silent. A failing "test_grep" will tell us "I was told to find THIS, but didn't find any in THAT".
Show 22 quoted lines
> + # bad whitespace > + printf "100644 $EMPTY_BLOB A" | > + test_must_fail git update-index --index-info 2>err && > + grep "malformed input line" err && > + > + # invalid stage value > + printf "100644 $EMPTY_BLOB 5\tA" | > + test_must_fail git update-index --index-info 2>err && > + grep "malformed input line" err && > + > + # invalid OID length > + printf "100755 abc123\tA" | > + test_must_fail git update-index --index-info 2>err && > + grep "malformed input line" err && > + > + # bad quoting > + printf "100644 $EMPTY_BLOB\t\"A" | > + test_must_fail git update-index --index-info 2>err && > + grep "bad quoting of path name" err > +' > + > test_done